Stage 2 experiment: Show Language Guide Before Solution #2574
Conversation
…ement-solution-step component
WalkthroughThis pull request introduces changes to the implementation of the solution and language guide display in the course stage tutorial card. The modifications involve adding a feature flag service to conditionally render the language guide before the solution based on user roles and specific flag settings. The changes enhance the component's flexibility by allowing different visibility rules for instructional content. Changes
Sequence DiagramsequenceDiagram
participant User
participant Component
participant FeatureFlagsService
User->>Component: Loads tutorial stage
Component->>FeatureFlagsService: Check language guide visibility
FeatureFlagsService-->>Component: Return visibility status
alt Language guide can be shown
Component->>User: Display Language Guide
end
alt Solution is available
Component->>User: Display Solution with Blurred Overlay
end
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs(1 hunks)app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.ts(2 hunks)app/services/feature-flags.ts(1 hunks)
🔇 Additional comments (4)
app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.ts (2)
5-5: LGTM! Service injection follows standard patterns.The import and service injection are implemented correctly following Ember.js conventions.
Also applies to: 7-7, 22-22
25-27: Add tests for the new feature flag functionality.The new computed property
canSeeLanguageGuideBeforeSolutionlacks test coverage. Since this controls a key feature of showing/hiding the language guide, it should be tested.Consider adding tests for:
- Staff user can see guide before solution
- Non-staff user with feature flag can see guide before solution
- Non-staff user without feature flag cannot see guide before solution
app/services/feature-flags.ts (1)
21-23: LGTM! Feature flag implementation follows established patterns.The new getter follows the service's existing patterns and properly handles type safety.
Add tests for the new feature flag.
Please add test coverage for this new feature flag getter to ensure it behaves correctly for:
- Staff users
- Users with the test feature flag
- Users without the feature flag
Verify feature flag naming convention.
The feature flag name
can-see-language-guide-before-solution-for-stage-2is quite long. Let's verify if it follows the project's naming convention.✅ Verification successful
Feature flag name follows established convention ✅
The feature flag name
can-see-language-guide-before-solution-for-stage-2perfectly matches the existing pattern used in the codebase, following the same structure ascan-see-split-up-git-commands-for-stage-1.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other feature flag names to compare naming patterns rg "getFeatureFlagValue\(['\"].*['\"]\)" --type tsLength of output: 387
app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs (1)
14-28: LGTM! Clear conditional structure for language guide.The template logic for displaying the language guide is clear and well-structured.
...ponents/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs
Outdated
Show resolved
Hide resolved
…in implement-solution-step
andy1li
force-pushed
the
stage-2-experiments
branch
from
6dee3bd to
ae4202a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs (1)
25-132: 🛠️ Refactor suggestionExtract duplicated solution display logic into a separate component.
The significant code duplication between the two solution display blocks should be eliminated as previously suggested.
🧹 Nitpick comments (1)
app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs (1)
34-44: Improve TypeScript type narrowing for solution property.The extra
if this.solutioncheck is a workaround for TypeScript type narrowing. Consider a more elegant solution:
- Use a computed property to handle the type narrowing:
get changedFilesWithSolution() { return this.solution?.changedFiles ?? []; }
- Update the template:
-{{#each this.solution.changedFiles as |changedFile|}} - {{! Extra if condition convinces typescript that solution isn't null }} - {{#if this.solution}} +{{#each this.changedFilesWithSolution as |changedFile|}} <FileDiffCard @filename={{changedFile.filename}} @code={{changedFile.diff}} @language={{this.solution.language.slug}} data-test-file-diff-card /> - {{/if}} {{/each}}Also applies to: 83-93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (1)
app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs (1)
14-23: LGTM! Clean implementation of the language guide section.The markup is well-structured and follows the project's conventions with proper test attributes.
| <button class="absolute inset-0" type="button" {{on "click" this.handleRevealSolutionButtonClick}} data-test-solution-blurred-overlay> | ||
| </button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility of the solution reveal button.
The button element lacks proper accessibility attributes:
- Missing aria-label
- Missing role attribute
- Empty button content
Apply this diff to improve accessibility:
-<button class="absolute inset-0" type="button" {{on "click" this.handleRevealSolutionButtonClick}} data-test-solution-blurred-overlay>
+<button
+ class="absolute inset-0"
+ type="button"
+ aria-label="Reveal solution"
+ role="button"
+ {{on "click" this.handleRevealSolutionButtonClick}}
+ data-test-solution-blurred-overlay
+>
+ <span class="sr-only">Reveal solution</span>
</button>Also applies to: 128-129
Checklist:
[percy]in the message to trigger)Summary by CodeRabbit
New Features
Improvements