Refactor feedback prompt component and integrate it into current step modal. #2335
Conversation
WalkthroughThe pull request involves significant restructuring of the feedback prompt component within the course page. Key changes include the removal of the Changes
Possibly related PRs
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Test Results 1 files ±0 1 suites ±0 6m 24s ⏱️ ±0s For more details on these errors, see this check. Results for commit fb2dbaf. ± Comparison against base commit da9cf02. |
❌ 16 Tests Failed:
View the top 3 failed tests by shortest run time
To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard |
Bundle ReportChanges will increase total bundle size by 64 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (9)
app/components/course-page/step-content.ts (1)
29-29: LGTM! Consider refactoring for improved maintainability.The addition of 'CourseStageStep' to hide the completion notice is logical and consistent with the existing pattern. However, as the list of step types grows, consider refactoring this method for better readability and maintainability.
Here's a suggested refactor using an array of step types:
private readonly stepTypesWithHiddenCompletionNotice = ['IntroductionStep', 'SetupStep', 'CourseStageStep']; get shouldHideCompletionNotice(): boolean { return this.stepTypesWithHiddenCompletionNotice.includes(this.args.step.type); }This approach would make it easier to add or remove step types in the future without modifying the method logic.
app/components/course-page/current-step-complete-modal.hbs (2)
7-19: LGTM! Consider adding a comment for clarity.The introduction of conditional rendering for the feedback prompt improves the component's flexibility and follows good separation of concerns. The changes look good overall.
Consider adding a brief comment explaining the purpose of the
shouldShowFeedbackPromptcondition, as it's not immediately clear from the template alone. This would improve code readability and maintainability.
Line range hint
1-54: Consider improving accessibility for the modal.The modal component looks well-structured and follows Ember.js best practices. To further enhance it, consider improving its accessibility:
- Add appropriate ARIA attributes to the modal for better screen reader support. For example:
Ensure that focus is properly managed when the modal opens and closes.
Add a visually hidden close button for keyboard users if one doesn't already exist in the parent component.
These changes will make the modal more accessible to users relying on assistive technologies.
app/components/course-page/course-stage-step/feedback-prompt.ts (2)
106-108: Approved: ConditionalonSubmitcallThe change correctly handles the new optional nature of
onSubmit. It prevents callingonSubmitif it's not provided or if editing a closed submission.For improved clarity, consider extracting the condition into a separate variable:
const shouldCallOnSubmit = this.args.onSubmit && !this.isEditingClosedSubmission; if (shouldCallOnSubmit) { this.args.onSubmit(); }This makes the intent clearer and could improve readability, especially if the condition becomes more complex in the future.
Line range hint
1-124: Consider additional improvementsWhile the changes made are consistent and work well together, there are a few areas in the file that could benefit from improvement:
- The
TODOcomments incongratulatoryMessagegetter (lines 48-52) should be addressed.- The
TODOcomment inhandleExplanationTextareaBlur(line 95) mentions a potential race condition that should be investigated.- Consider using TypeScript's nullish coalescing operator (
??) instead of logical OR (||) in thecongratulatoryMessagegetter (line 70) for more precise fallback behavior.Would you like assistance in addressing these
TODOcomments or implementing any of these suggestions? I can help draft the necessary changes or create GitHub issues to track these tasks.app/templates/course/stage/instructions.hbs (1)
72-75: Clarify the status of the prerequisites card.The prerequisites card section is currently commented out. If this functionality is no longer needed, it would be best to remove the commented code to maintain cleaner templates. If it's intended to be brought back in the future, consider moving it to a separate file or adding a more detailed TODO comment explaining the plan for this feature.
Could you please clarify the status of the prerequisites card? If it's no longer needed, I'd recommend removing the commented code.
app/components/course-page/current-step-complete-modal.ts (1)
1-7: Organize import statements for better readability.Consider grouping similar import statements together and ordering them from external modules to internal modules. This improves code readability and maintainability.
Here's how you might organize them:
import Component from '@glimmer/component'; import { action } from '@ember/object'; import { service } from '@ember/service'; +import type RouterService from '@ember/routing/router-service'; import CoursePageStateService from 'codecrafters-frontend/services/course-page-state'; import type Step from 'codecrafters-frontend/utils/course-page-step-list/step'; +import type CourseStageStep from 'codecrafters-frontend/utils/course-page-step-list/course-stage-step';app/components/course-page/course-stage-step/feedback-prompt.hbs (2)
Line range hint
39-47: Replace hardcoded username and link with dynamic valuesThe username 'rohitpaulk' and the link to the user's activity page are hardcoded in the template. This can lead to maintenance issues if the username changes or if the responsibility for reading feedback shifts to another user. Consider making the username and link dynamic by using properties from the component's context.
Apply this diff to make the username and link dynamic:
- <a - href="https://forum.codecrafters.io/u/rohitpaulk/activity" - target="_blank" - rel="noopener noreferrer" - class="underline font-semibold" - >rohitpaulk</a> + <a + href={{this.feedbackReaderLink}} + target="_blank" + rel="noopener noreferrer" + class="underline font-semibold" + >{{this.feedbackReaderName}}</a>Ensure that
this.feedbackReaderLinkandthis.feedbackReaderNameare defined in the component's JavaScript file.
Line range hint
49-60: Add an accessible label to theTextareacomponent instead of disabling the linterThe comment
{{! template-lint-disable require-input-label }}disables the linter rule enforcing accessible labels on input elements. For better accessibility and compliance with best practices, consider adding a proper label to theTextareacomponent rather than disabling the linter.Apply this change to add a label:
Remember to remove the linter disable comment and ensure the label text is localized by adding
feedbackPrompt.yourFeedbackto your localization files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- app/components/course-page/course-stage-step/feedback-prompt.hbs (4 hunks)
- app/components/course-page/course-stage-step/feedback-prompt.ts (2 hunks)
- app/components/course-page/current-step-complete-modal.hbs (1 hunks)
- app/components/course-page/current-step-complete-modal.ts (2 hunks)
- app/components/course-page/step-content.ts (1 hunks)
- app/controllers/course/stage/instructions.ts (0 hunks)
- app/templates/course/stage/instructions.hbs (1 hunks)
💤 Files with no reviewable changes (1)
- app/controllers/course/stage/instructions.ts
🧰 Additional context used
🔇 Additional comments (8)
app/components/course-page/step-content.ts (1)
29-29: Verify consistency with 'CourseStageStep' behavior.The change to hide the completion notice for 'CourseStageStep' seems logical, but it's important to ensure this is consistent with how 'CourseStageStep' is treated elsewhere in the application.
Please run the following script to check for other occurrences of 'CourseStageStep' and verify that this change aligns with its intended behavior:
This will help ensure that the change is consistent with the overall treatment of 'CourseStageStep' in the application.
app/components/course-page/course-stage-step/feedback-prompt.ts (1)
17-17:⚠️ Potential issueBreaking change:
onSubmitis now optionalThe
onSubmitproperty in theSignatureinterface has been made optional. This is a breaking change that might affect existing usage of the component whereonSubmitwas previously required.To ensure this change doesn't break existing code, please run the following script to check for any usages of this component where
onSubmitis still being passed:Additionally, please update the component's documentation to reflect this change and provide guidance on when
onSubmitshould be used.app/templates/course/stage/instructions.hbs (6)
4-10: Approve the improved badge awards handling.The change in the earned badge notice section improves the logic for displaying badge awards. Using
(gt this.badgeAwards.length 0)is a more explicit and potentially more efficient way to check for the presence of badge awards.
23-37: Acknowledge preservation of stage-specific tutorial cards.The first and second stage tutorial card sections have been retained without significant changes. This preserves the existing functionality for displaying stage-specific guidance to users, which is appropriate given the PR's focus on feedback prompt refactoring.
83-110: Approve the enhanced hints section with language filtering.The addition of a language filter toggle in the hints section is a valuable improvement to the user experience. It allows users to focus on hints relevant to their chosen language, while still providing the option to view all hints. The clear tooltips explaining the filter functionality are also a nice touch.
To ensure the language filter functionality works as intended, please run the following verification script:
#!/bin/bash # Description: Verify the language filter implementation # Test: Check for the presence of the language filter toggle rg --type handlebars '<Toggle.*handleCommentListFilterToggled' app/templates/course/stage/instructions.hbs # Test: Verify the implementation of the handleCommentListFilterToggled action ast-grep --lang javascript --pattern 'handleCommentListFilterToggled() { $$$ }' # Test: Check for the presence of the commentListIsFilteredByLanguage property ast-grep --lang javascript --pattern 'get commentListIsFilteredByLanguage() { $$$ }' # Test: Verify that the CommentList component receives the necessary parameters rg --type handlebars '<CoursePage::CommentList.*@shouldFilterByLanguage={{this\.commentListIsFilteredByLanguage}}' app/templates/course/stage/instructions.hbs
39-70: Approve the enhanced guidance for non-passed tests.The changes in this section significantly improve the user experience when tests have not passed. The addition of a context-sensitive popover with links to relevant guidance is a valuable enhancement. The conditional content based on test status (failed or other) provides more targeted assistance to users.
To ensure the popover functionality works as intended, please run the following verification script:
#!/bin/bash # Description: Verify the popover implementation and content # Test: Check for the presence of the EmberPopover component rg --type handlebars '<EmberPopover.*>' app/templates/course/stage/instructions.hbs # Test: Verify the conditional content within the popover rg --type handlebars 'eq this\.currentStep\.testsStatus "failed"' app/templates/course/stage/instructions.hbs # Test: Check for the presence of links to the "How to pass this stage" section rg --type handlebars 'href="#first-stage-tutorial-card".*How to pass this stage' app/templates/course/stage/instructions.hbs
12-21: Approve the refined test runner card display logic.The addition of
(eq this.currentStep.testsStatus "passed")to the conditional rendering of the test runner card improves the user experience by showing the card only when tests have passed. This change aligns well with the PR objectives.To ensure the test runner card behaves correctly with the new condition, please run the following verification script:
#!/bin/bash # Description: Verify the test runner card's conditional rendering # Test: Check for the presence of the condition in the template rg --type handlebars 'shouldShowTestRunnerCard.*eq.*currentStep\.testsStatus.*passed' app/templates/course/stage/instructions.hbs # Test: Verify the implementation of shouldShowTestRunnerCard computed property ast-grep --lang javascript --pattern 'get shouldShowTestRunnerCard() { $$$ }'
1-3: Approve the overlay component replacement.The replacement of
PreviousStepsIncompleteOverlaywithCurrentStepCompleteOverlayaligns with the PR objectives. This change shifts the focus from incomplete previous steps to the completion status of the current step, which should improve the user experience.To ensure the new component is properly implemented, please run the following verification script:
Also applies to: 112-112
| get currentStepAsCourseStageStep() { | ||
| return this.currentStep as CourseStageStep; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
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 shouldShowFeedbackPrompt() { | ||
| return this.currentStep.type === 'CourseStageStep' && !this.currentStepAsCourseStageStep.courseStage.isFirst; | ||
| } |
There was a problem hiding this comment.
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.
| // @ts-expect-error transitionTo types are wrong? | ||
| this.router.transitionTo(this.args.activeStep.routeParams.route, ...this.args.activeStep.routeParams.models); |
There was a problem hiding this comment.
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.
| <div class="prose dark:prose-invert leading-7"> | ||
| Feedback submitted! | ||
|
|
There was a problem hiding this comment.
🛠️ 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
Summary by CodeRabbit
New Features
Bug Fixes
Documentation