Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 8 additions & 28 deletions app/components/course-page/course-stage-step/feedback-prompt.hbs
Original file line number Diff line number Diff line change
@@ -1,24 +1,8 @@
<Alert @color="green" class="relative" data-test-feedback-prompt ...attributes>
{{#if this.isEditingClosedSubmission}}
<button
class="absolute right-4 top-4 opacity-60 hover:opacity-100"
type="button"
{{! @glint-expect-error fn/mut types don't work with false?}}
{{on "click" (fn (mut this.isEditingClosedSubmission) false)}}
data-test-close-feedback-prompt-button
>
{{svg-jar "x" class="w-5 h-5 text-green-600 dark:text-green-400"}}
</button>
{{/if}}

<div class="mr-2 mt-0.5 flex-shrink-0">
{{svg-jar "check-circle" class="w-6 fill-current text-teal-500"}}
</div>

<div class="relative" ...attributes>
<div class="w-full">
<div class="flex items-center">
{{#if (and this.submissionIsClosed (not this.isEditingClosedSubmission))}}
<div class="prose prose-green dark:prose-invert leading-7">
<div class="prose dark:prose-invert leading-7">
Feedback submitted!

<span
Comment on lines +5 to 7
Copy link
Contributor

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:

        <div class="prose dark:prose-invert leading-7">
-          Feedback submitted!
+          {{t 'feedbackPrompt.feedbackSubmitted'}}

And for lines 19-20:

          {{this.congratulatoryMessage}}
-          How did we do?
+          {{t 'feedbackPrompt.howDidWeDo'}}

Make sure that the translation keys feedbackPrompt.feedbackSubmitted and feedbackPrompt.howDidWeDo are added to your localization files.

Also applies to: 19-20

Expand All @@ -30,14 +14,10 @@
Edit Feedback
</span>
</div>
{{else}}
<div class="prose prose-green dark:prose-invert leading-7" data-test-question-text>
{{#if this.isEditingClosedSubmission}}
How did we do?
{{else}}
{{this.congratulatoryMessage}}
How did we do?
{{/if}}
{{else if (not this.isEditingClosedSubmission)}}
<div class="prose dark:prose-invert leading-7" data-test-question-text>
{{this.congratulatoryMessage}}
How did we do?
</div>
{{/if}}
</div>
Expand All @@ -56,7 +36,7 @@
</div>

{{#if this.feedbackSubmission.hasSelectedAnswer}}
<div class="prose prose-sm prose-green dark:prose-invert mt-4">
<div class="prose prose-sm dark:prose-invert mt-4">
P.S.
<a
href="https://forum.codecrafters.io/u/rohitpaulk/activity"
Expand Down Expand Up @@ -94,4 +74,4 @@
{{/if}}
{{/if}}
</div>
</Alert>
</div>
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ interface Signature {
Args: {
courseStage: CourseStageModel;
repository: RepositoryModel;
onSubmit: () => void;
onSubmit?: () => void;
};
}

Expand Down Expand Up @@ -103,7 +103,7 @@ export default class FeedbackPromptComponent extends Component<Signature> {
this.feedbackSubmission!.status = 'closed';

// Don't fire for editing submissions
if (!this.isEditingClosedSubmission) {
if (this.args.onSubmit && !this.isEditingClosedSubmission) {
this.args.onSubmit();
}

Expand Down
18 changes: 13 additions & 5 deletions app/components/course-page/current-step-complete-modal.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,19 @@
🎉&nbsp;&nbsp;{{if (eq this.currentStep.type "CourseStageStep") "Stage Completed!" "Step Completed!"}}
</div>

<div class="prose dark:prose-invert mb-8">
<p>
{{this.currentStep.completionNoticeMessage}}
</p>
</div>
{{#if this.shouldShowFeedbackPrompt}}
<CoursePage::CourseStageStep::FeedbackPrompt
@courseStage={{this.currentStepAsCourseStageStep.courseStage}}
@repository={{this.currentStepAsCourseStageStep.repository}}
class="mb-8"
/>
{{else}}
<div class="prose dark:prose-invert mb-8">
<p>
{{this.currentStep.completionNoticeMessage}}
</p>
</div>
{{/if}}

<div class="flex items-center gap-x-6 gap-y-4 flex-wrap">
<PrimaryLinkButton
Expand Down
19 changes: 19 additions & 0 deletions app/components/course-page/current-step-complete-modal.ts
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;

Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Consider using a type guard instead of type assertion.

Casting this.currentStep to CourseStageStep using as may lead to runtime errors if currentStep is not actually of type CourseStageStep. To ensure type safety, consider implementing a type guard that verifies the type before casting.

You can modify the currentStepAsCourseStageStep getter as follows:

- get currentStepAsCourseStageStep() {
-   return this.currentStep as CourseStageStep;
- }
+ get currentStepAsCourseStageStep(): CourseStageStep | undefined {
+   if (this.currentStep.type === 'CourseStageStep') {
+     return this.currentStep;
+   }
+   return undefined;
+ }

By returning undefined when the type check fails, you safeguard against potential runtime errors when accessing properties specific to CourseStageStep.

Committable suggestion was skipped due to low confidence.


get nextStep() {
return this.coursePageState.nextStep;
}

get shouldShowFeedbackPrompt() {
return this.currentStep.type === 'CourseStageStep' && !this.currentStepAsCourseStageStep.courseStage.isFirst;
}
Comment on lines +34 to +36
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle potential undefined value from currentStepAsCourseStageStep.

In shouldShowFeedbackPrompt, you're accessing this.currentStepAsCourseStageStep.courseStage.isFirst without checking if currentStepAsCourseStageStep is defined. This could cause a runtime error if currentStepAsCourseStageStep is undefined.

Update the shouldShowFeedbackPrompt getter to handle the possible undefined value:

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 courseStage when currentStep is valid.

Committable suggestion was skipped due to low confidence.


@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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Address the TypeScript error in transitionTo method call.

The comment // @ts-expect-error transitionTo types are wrong? suggests suppressing a TypeScript error. Suppressing errors can hide legitimate issues. It's better to resolve the type error to maintain type safety.

Investigate why this.router.transitionTo is causing a type error. It might be due to incorrect parameter types. Ensure that routeParams.route and routeParams.models match the expected types of transitionTo.

If routeParams.models is not an array of parameters or has the wrong type, you may need to adjust it or spread it correctly.

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 routeParams.models is an array. If models might be undefined, provide a default empty array to prevent errors.

Committable suggestion was skipped due to low confidence.

}
}

declare module '@glint/environment-ember-loose/registry' {
Expand Down
2 changes: 1 addition & 1 deletion app/components/course-page/step-content.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export default class StepContentComponent extends Component<Signature> {
@service declare coursePageState: CoursePageStateService;

get shouldHideCompletionNotice(): boolean {
return this.args.step.type === 'IntroductionStep' || this.args.step.type === 'SetupStep';
return this.args.step.type === 'IntroductionStep' || this.args.step.type === 'SetupStep' || this.args.step.type === 'CourseStageStep';
}

get shouldShowPreviousStepsIncompleteModal(): boolean {
Expand Down
4 changes: 0 additions & 4 deletions app/controllers/course/stage/instructions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,6 @@ export default class CourseStageInstructionsController extends Controller {
return this.model.courseStage.prerequisiteInstructionsMarkdownFor(this.model.activeRepository);
}

get shouldShowFeedbackPrompt() {
return !this.currentStep.courseStage.isFirst && this.currentStep.status === 'complete';
}

get shouldShowLanguageGuide() {
return !this.model.courseStage.isFirst && this.authenticator.currentUser?.isStaff;
}
Expand Down
189 changes: 93 additions & 96 deletions app/templates/course/stage/instructions.hbs
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>
Loading