Launch Terminal Instructions experiment for Shell stage 1 and 2 #2521
Conversation
WalkthroughThis pull request modifies the instructional content within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
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 (
|
andy1li
force-pushed
the
terminal-instructions-experiment
branch
2 times, most recently
from
7733897 to
8038953
Compare
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found. Additional details and impacted files📢 Thoughts on this report? Let us know! |
…d second stage tutorial cards and feature flag logic [percy]
andy1li
force-pushed
the
terminal-instructions-experiment
branch
from
8038953 to
02090bb
Compare
andy1li
changed the title
andy1li
force-pushed
the
terminal-instructions-experiment
branch
from
9429043 to
de8da26
Compare
|
Awesome work @andy1li! Like how we added interactivity. Wonder if we could make the availability of tooltips a bit more obvious - maybe by adding dotted underlines? No need to try right now, we can try some options on our call and finalize |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/components/course-page/course-stage-step/second-stage-tutorial-card.hbs (1)
16-23: Consider enhancing tooltip visibility.As suggested in the PR comments by
rohitpaulk, consider adding dotted underlines to make the tooltips more discoverable.- <p class="mt-0 mb-2 cursor-pointer w-fit text-slate-300 dark:text-slate-600">$ + <p class="mt-0 mb-2 cursor-pointer w-fit text-slate-300 dark:text-slate-600 border-b border-dotted border-slate-300 dark:border-slate-600">$app/components/course-page/course-stage-step/first-stage-tutorial-card.hbs (1)
25-28: Remove commented-out code.Clean up the commented-out question mark circle implementation.
- {{!-- <div class="flex items-center justify-center rounded-full relative"> - {{svg-jar "question-mark-circle" class="w-6 h-6 text-blue-500"}} - </div> --}}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/components/course-page/course-stage-step/first-stage-tutorial-card.hbs(1 hunks)app/components/course-page/course-stage-step/first-stage-tutorial-card.ts(1 hunks)app/components/course-page/course-stage-step/second-stage-tutorial-card.hbs(1 hunks)app/components/course-page/course-stage-step/second-stage-tutorial-card.ts(1 hunks)app/services/feature-flags.js(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test
app/components/course-page/course-stage-step/first-stage-tutorial-card.hbs
[error] 21-21: Object literal may only specify known properties, and 'popperContainer' does not exist in type 'NamedArgs'
🔇 Additional comments (5)
app/services/feature-flags.js (1)
17-19: LGTM! Clean implementation of the feature flag.The implementation follows the established patterns in the service and provides clear control over the terminal instructions experiment.
app/components/course-page/course-stage-step/second-stage-tutorial-card.hbs (1)
9-14: Nice touch with the terminal window styling!The implementation of the terminal window UI with the three dots header follows common terminal app patterns and supports dark mode well.
app/components/course-page/course-stage-step/second-stage-tutorial-card.ts (1)
86-88: LGTM! Clean getter implementation.The
shouldShowTerminalInstructionsgetter is well-implemented and properly typed.app/components/course-page/course-stage-step/first-stage-tutorial-card.ts (2)
88-90: LGTM! Clean implementation of the feature flag check.The computed property follows Ember.js conventions and is well-typed. The implementation aligns with the PR objectives to conditionally show terminal instructions for Shell stages.
88-90: Verify feature flag implementation and test coverage.Since this is an experimental feature, let's ensure proper implementation of the feature flag.
app/components/course-page/course-stage-step/first-stage-tutorial-card.hbs
Outdated
Show resolved
Hide resolved
app/components/course-page/course-stage-step/first-stage-tutorial-card.hbs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/components/course-page/course-stage-step/first-stage-tutorial-card.hbs (1)
4-4:⚠️ Potential issueReplace hardcoded condition with feature flag check.
The hardcoded
{{#if true}}defeats the purpose of the feature flag implementation.- {{#if true}} + {{#if this.shouldShowTerminalInstructions}}
🧹 Nitpick comments (1)
app/components/course-page/course-stage-step/first-stage-tutorial-card.hbs (1)
17-21: Consider enhancing tooltip discoverability.As suggested in the PR feedback, consider adding a dotted underline or similar visual indicator to make the tooltip's presence more apparent to users.
- <span class="relative flex size-6"> + <span class="relative flex size-6 border-b border-dotted border-slate-400 dark:border-slate-500"> <span class="animate-ping absolute inline-flex h-full w-full rounded-full bg-sky-400 opacity-75"></span> {{svg-jar "question-mark-circle" class="size-6 text-blue-500"}} </span>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/components/course-page/course-stage-step/first-stage-tutorial-card.hbs(1 hunks)app/components/course-page/course-stage-step/second-stage-tutorial-card.hbs(1 hunks)types/glint.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/course-page/course-stage-step/second-stage-tutorial-card.hbs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (2)
types/glint.d.ts (1)
22-22: LGTM! Type definition added for EmberTooltip's popperContainer.The addition of
popperContainer?: stringto EmberTooltip's type definition properly types the property and should resolve the pipeline failures.app/components/course-page/course-stage-step/first-stage-tutorial-card.hbs (1)
8-24: Well-structured terminal UI implementation!The terminal UI implementation follows best practices with:
- Semantic HTML structure
- Proper dark mode support
- Consistent spacing and styling
- Good accessibility considerations
…age-tutorial-card
…rst-stage-tutorial-card component
…ructions for first and second stage tutorials
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/components/course-page/course-stage-step/first-stage-tutorial-card.ts (3)
77-77: Consider adding JSDoc documentation.The tracked property is well-named and correctly implemented. Consider adding JSDoc documentation to explain its purpose and when it changes.
+ /** + * Tracks whether the user has clicked on the terminal instructions. + * Used to control the visibility of the pinging animation. + */ @tracked isTerminalInstructionsPingingClicked = false;
91-97: Add type safety for feature flags.The computed properties are well-implemented, but consider adding type safety for the feature flags to prevent runtime errors.
+ // In services/feature-flags.ts + interface FeatureFlags { + canSeeTerminalInstructionsForStage1And2: boolean; + // ... other flags + } + export default class FeatureFlagsService { + get canSeeTerminalInstructionsForStage1And2(): boolean { + // implementation + } + } // In this file get shouldShowTerminalInstructions() { - return this.featureFlags.canSeeTerminalInstructionsForStage1And2; + return Boolean(this.featureFlags.canSeeTerminalInstructionsForStage1And2); }
143-146: Add analytics tracking for terminal interactions.For consistency with other user interactions in this component, consider tracking this interaction in analytics.
@action handleTerminalClick() { this.isTerminalInstructionsPingingClicked = true; + this.analyticsEventTracker.track('terminal_instructions_clicked', { + repository_id: this.args.repository.id, + course_stage_id: this.args.courseStage.id + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/components/course-page/course-stage-step/first-stage-tutorial-card.hbs(1 hunks)app/components/course-page/course-stage-step/first-stage-tutorial-card.ts(4 hunks)app/components/course-page/course-stage-step/second-stage-tutorial-card.hbs(1 hunks)app/components/course-page/course-stage-step/second-stage-tutorial-card.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/components/course-page/course-stage-step/second-stage-tutorial-card.hbs
- app/components/course-page/course-stage-step/first-stage-tutorial-card.hbs
- app/components/course-page/course-stage-step/second-stage-tutorial-card.ts
🔇 Additional comments (1)
app/components/course-page/course-stage-step/first-stage-tutorial-card.ts (1)
12-12: LGTM!The import of
trackedis correctly placed and necessary for the new tracked property.
Bundle ReportChanges will increase total bundle size by 35.95MB (100.0%) ⬆️
|
Checklist:
[percy]in the message to trigger)Summary by CodeRabbit
New Features
Improvements
EmberTooltipcomponent to allow specification of a tooltip container.