-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor feedback prompt component and integrate it into current step modal. #2335
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,11 @@ | ||
import { action } from '@ember/object'; | ||
import { service } from '@ember/service'; | ||
import Component from '@glimmer/component'; | ||
import type Step from 'codecrafters-frontend/utils/course-page-step-list/step'; | ||
import CoursePageStateService from 'codecrafters-frontend/services/course-page-state'; | ||
import type CourseStageStep from 'codecrafters-frontend/utils/course-page-step-list/course-stage-step'; | ||
import type RouterService from '@ember/routing/router-service'; | ||
|
||
interface Signature { | ||
Element: HTMLDivElement; | ||
|
||
|
@@ -13,14 +17,29 @@ interface Signature { | |
|
||
export default class CurrentStepCompleteModalComponent extends Component<Signature> { | ||
@service declare coursePageState: CoursePageStateService; | ||
@service declare router: RouterService; | ||
|
||
get currentStep() { | ||
return this.coursePageState.currentStep; | ||
} | ||
|
||
get currentStepAsCourseStageStep() { | ||
return this.currentStep as CourseStageStep; | ||
} | ||
Comment on lines
+26
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider using a type guard instead of type assertion. Casting You can modify the - get currentStepAsCourseStageStep() {
- return this.currentStep as CourseStageStep;
- }
+ get currentStepAsCourseStageStep(): CourseStageStep | undefined {
+ if (this.currentStep.type === 'CourseStageStep') {
+ return this.currentStep;
+ }
+ return undefined;
+ } By returning
|
||
|
||
get nextStep() { | ||
return this.coursePageState.nextStep; | ||
} | ||
|
||
get shouldShowFeedbackPrompt() { | ||
return this.currentStep.type === 'CourseStageStep' && !this.currentStepAsCourseStageStep.courseStage.isFirst; | ||
} | ||
Comment on lines
+34
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle potential undefined value from In Update the get shouldShowFeedbackPrompt() {
- return this.currentStep.type === 'CourseStageStep' && !this.currentStepAsCourseStageStep.courseStage.isFirst;
+ const currentStep = this.currentStepAsCourseStageStep;
+ return currentStep ? !currentStep.courseStage.isFirst : false;
} This ensures that you only access
|
||
|
||
@action | ||
handleFeedbackPromptSubmit() { | ||
// @ts-expect-error transitionTo types are wrong? | ||
this.router.transitionTo(this.args.activeStep.routeParams.route, ...this.args.activeStep.routeParams.models); | ||
Comment on lines
+40
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Address the TypeScript error in The comment Investigate why If handleFeedbackPromptSubmit() {
- // @ts-expect-error transitionTo types are wrong?
this.router.transitionTo(
this.args.activeStep.routeParams.route,
- ...this.args.activeStep.routeParams.models
+ ...(this.args.activeStep.routeParams.models || [])
);
} Additionally, ensure that
|
||
} | ||
} | ||
|
||
declare module '@glint/environment-ember-loose/registry' { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,116 +1,113 @@ | ||
<CoursePage::PreviousStepsIncompleteOverlay @currentStep={{this.currentStep}}> | ||
<div class="pt-6 pb-32 px-3 md:px-6 lg:px-10" {{did-update this.handleDidUpdateTestsStatus this.currentStep.testsStatus}}> | ||
{{#if this.shouldShowUpgradePrompt}} | ||
<CoursePage::CourseStageStep::UpgradePrompt @featureSlugToHighlight="content" class="mb-6" /> | ||
{{/if}} | ||
<CoursePage::CurrentStepCompleteOverlay @currentStep={{this.currentStep}}> | ||
<div class="pt-6 pb-32 px-3 md:px-6 lg:px-10" {{did-update this.handleDidUpdateTestsStatus this.currentStep.testsStatus}}> | ||
{{#if this.shouldShowUpgradePrompt}} | ||
<CoursePage::CourseStageStep::UpgradePrompt @featureSlugToHighlight="content" class="mb-6" /> | ||
{{/if}} | ||
|
||
{{#if this.shouldShowFeedbackPrompt}} | ||
<CoursePage::CourseStageStep::FeedbackPrompt | ||
@courseStage={{@model.courseStage}} | ||
@repository={{@model.activeRepository}} | ||
@onSubmit={{this.handleStageFeedbackSubmitted}} | ||
class="mb-6" | ||
/> | ||
{{/if}} | ||
{{#if (gt this.badgeAwards.length 0)}} | ||
<CoursePage::CourseStageStep::EarnedBadgeNotice @badgeAwards={{this.badgeAwards}} class="mb-6" /> | ||
{{/if}} | ||
|
||
{{#if (gt this.badgeAwards.length 0)}} | ||
<CoursePage::CourseStageStep::EarnedBadgeNotice @badgeAwards={{this.badgeAwards}} class="mb-6" /> | ||
{{/if}} | ||
|
||
{{#if (and this.shouldShowTestRunnerCard (eq this.currentStep.testsStatus "passed"))}} | ||
<CoursePage::CourseStageStep::TestRunnerCard | ||
class="mb-6" | ||
id="test-runner-card" | ||
@isCollapsible={{false}} | ||
@repository={{@model.activeRepository}} | ||
@stage={{@model.courseStage}} | ||
{{upscroll-on-insert scrollableContainer="#course-page-scrollable-area"}} | ||
/> | ||
{{/if}} | ||
|
||
{{#if @model.courseStage.isFirst}} | ||
<CoursePage::CourseStageStep::FirstStageTutorialCard @repository={{@model.activeRepository}} @courseStage={{@model.courseStage}} class="mb-6" /> | ||
{{/if}} | ||
|
||
{{#if @model.courseStage.isSecond}} | ||
<CoursePage::CourseStageStep::SecondStageInstructionsCard | ||
@repository={{@model.activeRepository}} | ||
@courseStage={{@model.courseStage}} | ||
class="mb-6" | ||
/> | ||
{{/if}} | ||
|
||
{{#if (and this.shouldShowTestRunnerCard (not-eq this.currentStep.testsStatus "passed"))}} | ||
<div class="mb-6"> | ||
{{#if (and this.shouldShowTestRunnerCard (eq this.currentStep.testsStatus "passed"))}} | ||
<CoursePage::CourseStageStep::TestRunnerCard | ||
{{! template-lint-disable no-duplicate-id }} | ||
class="mb-6" | ||
id="test-runner-card" | ||
@isCollapsible={{or this.shouldSuppressTestRunnerCardExpands (not-eq this.currentStep.testsStatus "passed")}} | ||
@isCollapsible={{false}} | ||
@repository={{@model.activeRepository}} | ||
@stage={{@model.courseStage}} | ||
@onExpand={{if this.shouldSuppressTestRunnerCardExpands this.handleTestRunnerCardExpandedOnFirstStage undefined}} | ||
{{upscroll-on-insert scrollableContainer="#course-page-scrollable-area"}} | ||
/> | ||
{{/if}} | ||
|
||
{{#if this.shouldSuppressTestRunnerCardExpands}} | ||
<EmberPopover @popperContainer="body"> | ||
<div class="prose prose-sm prose-invert text-white"> | ||
{{#if (eq this.currentStep.testsStatus "failed")}} | ||
<p> | ||
This failure is expected! Check the | ||
<a href="#first-stage-tutorial-card">How to pass this stage</a> | ||
section for instructions. | ||
</p> | ||
{{else}} | ||
<p> | ||
Check the | ||
<a href="#first-stage-tutorial-card">How to pass this stage</a> | ||
section for instructions. | ||
</p> | ||
{{/if}} | ||
</div> | ||
</EmberPopover> | ||
{{/if}} | ||
</div> | ||
{{/if}} | ||
{{#if @model.courseStage.isFirst}} | ||
<CoursePage::CourseStageStep::FirstStageTutorialCard | ||
@repository={{@model.activeRepository}} | ||
@courseStage={{@model.courseStage}} | ||
class="mb-6" | ||
/> | ||
{{/if}} | ||
|
||
{{! TODO: Bring this back? }} | ||
{{!-- {{#if this.shouldShowPrerequisites}} | ||
{{#if @model.courseStage.isSecond}} | ||
<CoursePage::CourseStageStep::SecondStageInstructionsCard | ||
@repository={{@model.activeRepository}} | ||
@courseStage={{@model.courseStage}} | ||
class="mb-6" | ||
/> | ||
{{/if}} | ||
|
||
{{#if (and this.shouldShowTestRunnerCard (not-eq this.currentStep.testsStatus "passed"))}} | ||
<div class="mb-6"> | ||
<CoursePage::CourseStageStep::TestRunnerCard | ||
{{! template-lint-disable no-duplicate-id }} | ||
id="test-runner-card" | ||
@isCollapsible={{or this.shouldSuppressTestRunnerCardExpands (not-eq this.currentStep.testsStatus "passed")}} | ||
@repository={{@model.activeRepository}} | ||
@stage={{@model.courseStage}} | ||
@onExpand={{if this.shouldSuppressTestRunnerCardExpands this.handleTestRunnerCardExpandedOnFirstStage undefined}} | ||
/> | ||
|
||
{{#if this.shouldSuppressTestRunnerCardExpands}} | ||
<EmberPopover @popperContainer="body"> | ||
<div class="prose prose-sm prose-invert text-white"> | ||
{{#if (eq this.currentStep.testsStatus "failed")}} | ||
<p> | ||
This failure is expected! Check the | ||
<a href="#first-stage-tutorial-card">How to pass this stage</a> | ||
section for instructions. | ||
</p> | ||
{{else}} | ||
<p> | ||
Check the | ||
<a href="#first-stage-tutorial-card">How to pass this stage</a> | ||
section for instructions. | ||
</p> | ||
{{/if}} | ||
</div> | ||
</EmberPopover> | ||
{{/if}} | ||
</div> | ||
{{/if}} | ||
|
||
{{! TODO: Bring this back? }} | ||
{{!-- {{#if this.shouldShowPrerequisites}} | ||
<CoursePage::CourseStageStep::PrerequisitesCard @repository={{@model.activeRepository}} @courseStage={{@model.courseStage}} class="mb-6" /> | ||
{{/if}} --}} | ||
|
||
<CoursePage::CourseStageStep::YourTaskCard @repository={{@model.activeRepository}} @courseStage={{@model.courseStage}} /> | ||
<CoursePage::CourseStageStep::YourTaskCard @repository={{@model.activeRepository}} @courseStage={{@model.courseStage}} /> | ||
|
||
{{#if this.shouldShowLanguageGuide}} | ||
<CoursePage::CourseStageStep::LanguageGuideCard @repository={{@model.activeRepository}} @courseStage={{@model.courseStage}} class="mt-6" /> | ||
{{/if}} | ||
{{#if this.shouldShowLanguageGuide}} | ||
<CoursePage::CourseStageStep::LanguageGuideCard @repository={{@model.activeRepository}} @courseStage={{@model.courseStage}} class="mt-6" /> | ||
{{/if}} | ||
|
||
<div data-percy-hints-section> | ||
<div class="border-b dark:border-white/5 pb-2 mb-6 mt-8 flex items-center justify-between"> | ||
<h2 class="font-semibold text-lg text-gray-800 dark:text-gray-200">Hints</h2> | ||
<div data-percy-hints-section> | ||
<div class="border-b dark:border-white/5 pb-2 mb-6 mt-8 flex items-center justify-between"> | ||
<h2 class="font-semibold text-lg text-gray-800 dark:text-gray-200">Hints</h2> | ||
|
||
{{#if @model.activeRepository.language}} | ||
<div class="flex items-center gap-1"> | ||
<span class="text-xs text-gray-500 mr-2">Filter by {{@model.activeRepository.language.name}}</span> | ||
<Toggle @isOn={{this.commentListIsFilteredByLanguage}} {{on "click" this.handleCommentListFilterToggled}} /> | ||
{{#if @model.activeRepository.language}} | ||
<div class="flex items-center gap-1"> | ||
<span class="text-xs text-gray-500 mr-2">Filter by {{@model.activeRepository.language.name}}</span> | ||
<Toggle @isOn={{this.commentListIsFilteredByLanguage}} {{on "click" this.handleCommentListFilterToggled}} /> | ||
|
||
<EmberTooltip> | ||
{{#if this.commentListIsFilteredByLanguage}} | ||
You're currently viewing hints that are relevant to | ||
{{@model.activeRepository.language.name}}. Turn this off to view hints for all languages. | ||
{{else}} | ||
You're currently viewing hints for all languages. Turn this on to filter hints that are relevant to | ||
{{@model.activeRepository.language.name}}. | ||
{{/if}} | ||
</EmberTooltip> | ||
</div> | ||
{{/if}} | ||
</div> | ||
<EmberTooltip> | ||
{{#if this.commentListIsFilteredByLanguage}} | ||
You're currently viewing hints that are relevant to | ||
{{@model.activeRepository.language.name}}. Turn this off to view hints for all languages. | ||
{{else}} | ||
You're currently viewing hints for all languages. Turn this on to filter hints that are relevant to | ||
{{@model.activeRepository.language.name}}. | ||
{{/if}} | ||
</EmberTooltip> | ||
</div> | ||
{{/if}} | ||
</div> | ||
|
||
<CoursePage::CommentList | ||
@courseStage={{@model.courseStage}} | ||
@language={{@model.activeRepository.language}} | ||
@shouldFilterByLanguage={{this.commentListIsFilteredByLanguage}} | ||
/> | ||
<CoursePage::CommentList | ||
@courseStage={{@model.courseStage}} | ||
@language={{@model.activeRepository.language}} | ||
@shouldFilterByLanguage={{this.commentListIsFilteredByLanguage}} | ||
/> | ||
</div> | ||
</div> | ||
</div> | ||
</CoursePage::CurrentStepCompleteOverlay> | ||
</CoursePage::PreviousStepsIncompleteOverlay> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Wrap static text strings in translation helpers for localization
The strings "Feedback submitted!" and "How did we do?" are hardcoded in the template. To support localization and make the application accessible to users in different languages, consider wrapping these strings in translation helpers.
Example fix for lines 5-7:
And for lines 19-20:
Make sure that the translation keys
feedbackPrompt.feedbackSubmitted
andfeedbackPrompt.howDidWeDo
are added to your localization files.Also applies to: 19-20