Refactor and enhance question card option with confetti animation #2651
Conversation
WalkthroughThe changes update the question card options in the concept page by integrating confetti animation for correct answers. The button template now uses a helper to trigger the confetti effect upon insertion. The previous JavaScript component is removed and replaced with a new TypeScript component that defines computed properties for the option state and an action to invoke the confetti service. Additionally, the parent question card component is modified to pass confetti state and callback bindings to the option component, and it now tracks whether confetti has been shown. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant QC as QuestionCard (Container)
participant QCO as QuestionCardOption Component
participant CS as ConfettiService
U->>QC: Render question card
QC->>QCO: Pass @questionSlug, @hasShownConfetti, @onConfettiShown
QCO->>QCO: did-insert triggers fireCorrectAnswerConfetti()
alt Correct answer & confetti not shown
QCO->>CS: Request confetti animation
CS-->>QCO: Animation completed
QCO->>QC: Invoke onConfettiShown callback
else Already shown or incorrect
QCO-->>QC: No animation triggered
end
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 26s ⏱️ +10s For more details on these errors, see this check. Results for commit fab1d7c. ± Comparison against base commit ab2dbbb. This pull request removes 617 and adds 617 tests. Note that renamed tests count towards both.This pull request removes 41 skipped tests and adds 41 skipped tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Bundle ReportChanges will increase total bundle size by 3.04kB (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: client-array-pushAssets Changed:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/services/confetti.ts (1)
30-54: Add blank lines for better code readability.The method implementation looks good, but add blank lines before conditional statements to improve readability.
const mouseEvent = window.event as MouseEvent; + if (mouseEvent) { const x = mouseEvent.clientX; const y = mouseEvent.clientY; + if (x > rect.left && x < rect.right && y > rect.top && y < rect.bottom) { return {🧰 Tools
🪛 ESLint
[error] 40-49: Expected blank line before this statement.
(padding-line-between-statements)
[error] 43-48: Expected blank line before this statement.
(padding-line-between-statements)
🪛 GitHub Actions: Test
[error] 40-40: Expected blank line before this statement padding-line-between-statements
[error] 43-43: Expected blank line before this statement padding-line-between-statements
app/components/concept-page/question-card-option.ts (1)
65-80: Consider adding error handling for confetti animation.The implementation looks good, but consider handling potential failures in the confetti animation.
@action async fireCorrectAnswerConfetti(element: HTMLElement) { if (this.hasShownConfetti || !this.isSelectedAndCorrect) { return; } this.hasShownConfetti = true; this.localStorage.setItem(this.storageKey, 'true'); - await this.confetti.fireFromMousePositionOrElement(element, { - particleCount: 50, - spread: 60, - startVelocity: 20, - colors: ['#22c55e', '#16a34a', '#15803d'], // green colors - disableForReducedMotion: true, - }); + try { + await this.confetti.fireFromMousePositionOrElement(element, { + particleCount: 50, + spread: 60, + startVelocity: 20, + colors: ['#22c55e', '#16a34a', '#15803d'], // green colors + disableForReducedMotion: true, + }); + } catch (error) { + console.error('Failed to show confetti animation:', error); + // Animation failure shouldn't affect the user experience + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/components/concept-page/question-card-option.hbs(1 hunks)app/components/concept-page/question-card-option.js(0 hunks)app/components/concept-page/question-card-option.ts(1 hunks)app/components/concept-page/question-card.hbs(1 hunks)app/services/confetti.ts(2 hunks)
💤 Files with no reviewable changes (1)
- app/components/concept-page/question-card-option.js
🧰 Additional context used
🪛 ESLint
app/services/confetti.ts
[error] 40-49: Expected blank line before this statement.
(padding-line-between-statements)
[error] 43-48: Expected blank line before this statement.
(padding-line-between-statements)
🪛 GitHub Actions: Test
app/services/confetti.ts
[error] 40-40: Expected blank line before this statement padding-line-between-statements
[error] 43-43: Expected blank line before this statement padding-line-between-statements
🔇 Additional comments (4)
app/services/confetti.ts (1)
30-54: Consider adding type safety for MouseEvent.The implementation looks good, but there's a potential type safety concern with
window.event.
window.eventis deprecated and not available in all browsers- Type casting to
MouseEventmight failConsider using the following pattern instead:
-const mouseEvent = window.event as MouseEvent; +const mouseEvent = window.event instanceof MouseEvent ? window.event : null;Also applies to: 75-84
🧰 Tools
🪛 ESLint
[error] 40-49: Expected blank line before this statement.
(padding-line-between-statements)
[error] 43-48: Expected blank line before this statement.
(padding-line-between-statements)
🪛 GitHub Actions: Test
[error] 40-40: Expected blank line before this statement padding-line-between-statements
[error] 43-43: Expected blank line before this statement padding-line-between-statements
app/components/concept-page/question-card-option.ts (1)
8-21: LGTM! Well-structured TypeScript interface.The interface definition is comprehensive and properly typed.
app/components/concept-page/question-card-option.hbs (1)
8-9: LGTM! Clean integration of confetti animation.The did-insert helper is properly integrated with the TypeScript component.
app/components/concept-page/question-card.hbs (1)
14-14: LGTM! Proper data flow for confetti state persistence.The questionSlug prop is correctly passed to enable confetti state persistence per question.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/services/confetti.ts (1)
77-86: Add JSDoc documentation and update method signature.The method needs documentation and should be updated to work with the suggested changes to the calculation method.
Add documentation and update the signature:
+ /** + * Fires confetti from either the mouse position (if mouse is over the element) + * or from the element's center. + * @param element - The HTML element to use as reference + * @param mouseEvent - Optional mouse event to determine confetti origin + * @param options - Optional confetti configuration options + * @returns Promise that resolves when confetti animation starts + */ - async fireFromMousePositionOrElement(element: HTMLElement, options: confetti.Options = {}) { + async fireFromMousePositionOrElement( + element: HTMLElement, + mouseEvent?: MouseEvent, + options: confetti.Options = {} + ) { if (this.fastboot.isFastBoot) { return; } - options.origin = this.#calculateConfettiOriginBasedOnMousePositionOrElementCenter(element); + options.origin = this.#calculateConfettiOriginBasedOnMousePositionOrElementCenter(element, mouseEvent); return confetti(options); }The lack of test coverage for this method needs to be addressed. Would you like me to help generate test cases?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 80-80: app/services/confetti.ts#L80
Added line #L80 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/services/confetti.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/services/confetti.ts
[warning] 42-43: app/services/confetti.ts#L42-L43
Added lines #L42 - L43 were not covered by tests
[warning] 46-46: app/services/confetti.ts#L46
Added line #L46 was not covered by tests
[warning] 80-80: app/services/confetti.ts#L80
Added line #L80 was not covered by tests
app/services/confetti.ts
Outdated
| #calculateConfettiOriginBasedOnMousePositionOrElementCenter(element: HTMLElement) { | ||
| // Get viewport dimensions | ||
| const viewportWidth = window.innerWidth; | ||
| const viewportHeight = window.innerHeight; | ||
|
|
||
| // Get the element's bounding rectangle | ||
| const rect = element.getBoundingClientRect(); | ||
|
|
||
| // if mouseEvent is inside the rect, return the mouseEvent's position | ||
| const mouseEvent = window.event as MouseEvent; | ||
|
|
||
| if (mouseEvent) { | ||
| const x = mouseEvent.clientX; | ||
| const y = mouseEvent.clientY; | ||
|
|
||
| if (x > rect.left && x < rect.right && y > rect.top && y < rect.bottom) { | ||
| return { | ||
| x: x / viewportWidth, | ||
| y: y / viewportHeight, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| // otherwise, return the rect's center position | ||
| return this.#calculateConfettiOrigin(element); | ||
| } |
There was a problem hiding this comment.
Replace window.event with an explicit event parameter for better browser compatibility.
The current implementation has several concerns:
- Using
window.eventis non-standard and may not be supported in all browsers - Type casting
window.event as MouseEventwithout null checks is unsafe
Consider refactoring to accept an explicit MouseEvent parameter:
- #calculateConfettiOriginBasedOnMousePositionOrElementCenter(element: HTMLElement) {
+ #calculateConfettiOriginBasedOnMousePositionOrElementCenter(element: HTMLElement, mouseEvent?: MouseEvent) {
const viewportWidth = window.innerWidth;
const viewportHeight = window.innerHeight;
const rect = element.getBoundingClientRect();
- const mouseEvent = window.event as MouseEvent;
-
if (mouseEvent) {
const x = mouseEvent.clientX;
const y = mouseEvent.clientY;The lack of test coverage for this method needs to be addressed. Would you like me to help generate test cases?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #calculateConfettiOriginBasedOnMousePositionOrElementCenter(element: HTMLElement) { | |
| // Get viewport dimensions | |
| const viewportWidth = window.innerWidth; | |
| const viewportHeight = window.innerHeight; | |
| // Get the element's bounding rectangle | |
| const rect = element.getBoundingClientRect(); | |
| // if mouseEvent is inside the rect, return the mouseEvent's position | |
| const mouseEvent = window.event as MouseEvent; | |
| if (mouseEvent) { | |
| const x = mouseEvent.clientX; | |
| const y = mouseEvent.clientY; | |
| if (x > rect.left && x < rect.right && y > rect.top && y < rect.bottom) { | |
| return { | |
| x: x / viewportWidth, | |
| y: y / viewportHeight, | |
| }; | |
| } | |
| } | |
| // otherwise, return the rect's center position | |
| return this.#calculateConfettiOrigin(element); | |
| } | |
| #calculateConfettiOriginBasedOnMousePositionOrElementCenter(element: HTMLElement, mouseEvent?: MouseEvent) { | |
| // Get viewport dimensions | |
| const viewportWidth = window.innerWidth; | |
| const viewportHeight = window.innerHeight; | |
| // Get the element's bounding rectangle | |
| const rect = element.getBoundingClientRect(); | |
| // if mouseEvent is inside the rect, return the mouseEvent's position | |
| if (mouseEvent) { | |
| const x = mouseEvent.clientX; | |
| const y = mouseEvent.clientY; | |
| if (x > rect.left && x < rect.right && y > rect.top && y < rect.bottom) { | |
| return { | |
| x: x / viewportWidth, | |
| y: y / viewportHeight, | |
| }; | |
| } | |
| } | |
| // otherwise, return the rect's center position | |
| return this.#calculateConfettiOrigin(element); | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 42-43: app/services/confetti.ts#L42-L43
Added lines #L42 - L43 were not covered by tests
[warning] 46-46: app/services/confetti.ts#L46
Added line #L46 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/components/concept-page/question-card-option.ts (2)
23-52: Consider optimizing computed properties.The computed properties are well-organized but could be simplified to improve readability and reduce duplication.
Consider this refactoring:
- get isSelectedAndCorrect() { - return this.args.isSubmitted && this.args.option.isSelected && this.args.option.is_correct; - } - - get isSelectedAndUnsubmitted() { - return !this.args.isSubmitted && this.args.option.isSelected; - } - - get isSelectedAndWrong() { - return this.args.isSubmitted && this.args.option.isSelected && !this.args.option.is_correct; - } - - get isSubmitted() { - return this.args.isSubmitted; - } - - get isUnselected() { - return !this.args.option.isSelected; - } - - get isUnselectedAndCorrect() { - return this.args.isSubmitted && !this.args.option.isSelected && this.args.option.is_correct; - } + // Base states + get isSubmitted() { + return this.args.isSubmitted; + } + + get isSelected() { + return this.args.option.isSelected; + } + + // Derived states + get isSelectedAndCorrect() { + return this.isSubmitted && this.isSelected && this.isCorrect; + } + + get isSelectedAndUnsubmitted() { + return !this.isSubmitted && this.isSelected; + } + + get isSelectedAndWrong() { + return this.isSubmitted && this.isSelected && !this.isCorrect; + } + + get isUnselectedAndCorrect() { + return this.isSubmitted && !this.isSelected && this.isCorrect; + }
54-70: Remove debug logging and extract confetti configuration.The confetti action is well-implemented, but has two areas for improvement:
- Remove the console.log statement
- Extract the hardcoded confetti configuration
Consider this refactoring:
+ // Define confetti configuration as a static property or move to a config file + static CONFETTI_CONFIG = { + particleCount: 50, + spread: 60, + startVelocity: 20, + colors: ['#22c55e', '#16a34a', '#15803d'], // green colors + disableForReducedMotion: true, + }; + @action async fireCorrectAnswerConfetti(element: HTMLElement) { - console.log('fireCorrectAnswerConfetti', this.args.hasShownConfetti, this.isSelectedAndCorrect); if (this.args.hasShownConfetti || !this.isSelectedAndCorrect) { return; } this.args.onConfettiShown(); - await this.confetti.fireFromElement(element, { - particleCount: 50, - spread: 60, - startVelocity: 20, - colors: ['#22c55e', '#16a34a', '#15803d'], // green colors - disableForReducedMotion: true, - }); + await this.confetti.fireFromElement(element, QuestionCardOptionComponent.CONFETTI_CONFIG); }🧰 Tools
🪛 ESLint
[error] 57-59: Expected blank line before this statement.
(padding-line-between-statements)
app/components/concept-page/question-card.ts (1)
144-149: Consider reordering methods for better organization.The method implementation is correct, but it should be moved before
handleShowExplanationClickto maintain a consistent order of class members.Apply this diff to reorder the methods:
- @action - handleShowExplanationClick() { - this.handleOptionSelected(this.args.question.correctOptionIndex); - } - @action handleConfettiShown() { next(() => { this._hasShownConfetti = true; }); } + + @action + handleShowExplanationClick() { + this.handleOptionSelected(this.args.question.correctOptionIndex); + }🧰 Tools
🪛 ESLint
[error] 144-149: Member handleConfettiShown should be declared before member handleShowExplanationClick.
(@typescript-eslint/member-ordering)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/components/concept-page/question-card-option.ts(1 hunks)app/components/concept-page/question-card.hbs(1 hunks)app/components/concept-page/question-card.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/concept-page/question-card.hbs
🧰 Additional context used
🪛 ESLint
app/components/concept-page/question-card.ts
[error] 144-149: Member handleConfettiShown should be declared before member handleShowExplanationClick.
(@typescript-eslint/member-ordering)
app/components/concept-page/question-card-option.ts
[error] 57-59: Expected blank line before this statement.
(padding-line-between-statements)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (3)
app/components/concept-page/question-card-option.ts (2)
1-21: LGTM! Well-structured TypeScript interface.The interface is well-defined with proper types for all props, and the imports are comprehensive.
73-77: LGTM! Proper Glint registry declaration.The component is correctly registered for Glint type checking.
app/components/concept-page/question-card.ts (1)
19-23: LGTM! Well-structured state management.The confetti state is properly managed using a tracked property with a clear public getter.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/concept-page/question-card-option.ts (1)
6-21: Consider using more specific types in the interface.The interface is well-structured, but could benefit from more specific types:
- Consider using a discriminated union type for the option state
- Add readonly modifiers to prevent accidental mutations
export interface Signature { Element: HTMLButtonElement; Args: { - option: { - is_correct: boolean; - isSelected: boolean; - markdown: string; - explanation_markdown: string; + readonly option: { + readonly is_correct: boolean; + readonly isSelected: boolean; + readonly markdown: string; + readonly explanation_markdown: string; }; - isSubmitted: boolean; - questionSlug: string; - hasShownConfetti: boolean; - onConfettiShown: () => void; + readonly isSubmitted: boolean; + readonly questionSlug: string; + readonly hasShownConfetti: boolean; + readonly onConfettiShown: () => void; }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/components/concept-page/question-card-option.ts(1 hunks)app/components/concept-page/question-card.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/concept-page/question-card.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (2)
app/components/concept-page/question-card-option.ts (2)
23-53: Well-structured computed properties!The component's state management is well-organized with clear, single-responsibility computed properties that make the component's behavior easy to understand and maintain.
74-78: LGTM!The Glint registry entry is properly configured, enabling type checking in templates.
| @action | ||
| async fireCorrectAnswerConfetti(element: HTMLElement) { | ||
| console.log('fireCorrectAnswerConfetti', this.args.hasShownConfetti, this.isSelectedAndCorrect); | ||
|
|
||
| if (this.args.hasShownConfetti || !this.isSelectedAndCorrect) { | ||
| return; | ||
| } | ||
|
|
||
| this.args.onConfettiShown(); | ||
|
|
||
| await this.confetti.fireFromElement(element, { | ||
| particleCount: 50, | ||
| spread: 60, | ||
| startVelocity: 20, | ||
| colors: ['#22c55e', '#16a34a', '#15803d'], // green colors | ||
| disableForReducedMotion: true, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clean up confetti implementation.
A few improvements could be made to the confetti implementation:
- Remove the console.log statement
- Consider extracting magic numbers into named constants
- Add error handling for the confetti service call
+ private readonly CONFETTI_CONFIG = {
+ particleCount: 50,
+ spread: 60,
+ startVelocity: 20,
+ colors: ['#22c55e', '#16a34a', '#15803d'], // green colors
+ disableForReducedMotion: true,
+ } as const;
@action
async fireCorrectAnswerConfetti(element: HTMLElement) {
- console.log('fireCorrectAnswerConfetti', this.args.hasShownConfetti, this.isSelectedAndCorrect);
-
if (this.args.hasShownConfetti || !this.isSelectedAndCorrect) {
return;
}
this.args.onConfettiShown();
- await this.confetti.fireFromElement(element, {
- particleCount: 50,
- spread: 60,
- startVelocity: 20,
- colors: ['#22c55e', '#16a34a', '#15803d'], // green colors
- disableForReducedMotion: true,
- });
+ try {
+ await this.confetti.fireFromElement(element, this.CONFETTI_CONFIG);
+ } catch (error) {
+ console.error('Failed to show confetti animation:', error);
+ // Animation failure shouldn't break the app
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @action | |
| async fireCorrectAnswerConfetti(element: HTMLElement) { | |
| console.log('fireCorrectAnswerConfetti', this.args.hasShownConfetti, this.isSelectedAndCorrect); | |
| if (this.args.hasShownConfetti || !this.isSelectedAndCorrect) { | |
| return; | |
| } | |
| this.args.onConfettiShown(); | |
| await this.confetti.fireFromElement(element, { | |
| particleCount: 50, | |
| spread: 60, | |
| startVelocity: 20, | |
| colors: ['#22c55e', '#16a34a', '#15803d'], // green colors | |
| disableForReducedMotion: true, | |
| }); | |
| } | |
| private readonly CONFETTI_CONFIG = { | |
| particleCount: 50, | |
| spread: 60, | |
| startVelocity: 20, | |
| colors: ['#22c55e', '#16a34a', '#15803d'], // green colors | |
| disableForReducedMotion: true, | |
| } as const; | |
| @action | |
| async fireCorrectAnswerConfetti(element: HTMLElement) { | |
| if (this.args.hasShownConfetti || !this.isSelectedAndCorrect) { | |
| return; | |
| } | |
| this.args.onConfettiShown(); | |
| try { | |
| await this.confetti.fireFromElement(element, this.CONFETTI_CONFIG); | |
| } catch (error) { | |
| console.error('Failed to show confetti animation:', error); | |
| // Animation failure shouldn't break the app | |
| } | |
| } |
| {{if this.isSelectedAndCorrect 'bg-green-200'}} | ||
| " | ||
| {{if this.isSelectedAndCorrect 'bg-green-200'}}" | ||
| {{did-insert this.fireCorrectAnswerConfetti}} |
There was a problem hiding this comment.
Let's fire confetti from the "Success" message, and not from the option itself
| @@ -16,11 +16,16 @@ interface Signature { | |||
|
|
|||
| export default class QuestionCardComponent extends Component<Signature> { | |||
| @tracked selectedOptionIndex: number | null = null; | |||
| @tracked _hasShownConfetti = false; | |||
There was a problem hiding this comment.
| @tracked _hasShownConfetti = false; | |
| @tracked hasShownConfetti = false; |
We don't need a separate underscored version
Convert the question card option component to TypeScript, add a confetti animation for correct answers, and improve the component's functionality by persisting the confetti state and calculating the confetti origin based on mouse position or element center.
Checklist:
[percy]in the message to trigger)Summary by CodeRabbit
New Features
QuestionCardOptioncomponent with additional properties for improved context and interaction.Refactor