Fix CCJ-123: add "close" button to victory modal #7764
Conversation
Also makes it so that keyboard focus goes to "continue" button in victory modal, which lets you press escape to close the modal, instead of staying in the ace editor. Fixes a random extra box shadow in CourseVictoryModal that was causing minor visual glitching. Trivial refactor to some logging code that was decaffeinated poorly and logging in two cases instead of one.
WalkthroughThis pull request introduces modifications to the styling and functionality of victory modals in the application. Key changes include the addition of a close button in the hero victory modal, updates to SASS styles for both course and hero victory modals, and improvements to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
app/styles/play/level/modal/course-victory-modal.sass (1)
Close Button Not Implemented
The shell scripts did not find any instances of
id="close-modal"in the HTML or JavaScript files, nor were there recent commits related toclose-modal. It appears that the close button referenced in the PR objectives has not been implemented. Please ensure that the close button is added to the HTML template and that appropriate JavaScript event listeners are in place.🔗 Analysis chain
Line range hint
52-59: Verify the implementation of the close button.I notice that there's an existing style for
#close-modalin this file. Could you please confirm if this style is being used for the newly added close button mentioned in the PR objectives? If so, it might be worth reviewing if the positioning and styling (e.g., color, padding) are still appropriate for the new implementation.To ensure the close button is properly implemented and styled, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation and usage of the close button in the victory modal. # Test 1: Check for the presence of a close button in the HTML template echo "Checking for close button in HTML template:" rg --type html 'id="close-modal"' -A 5 -B 5 # Test 2: Verify if there are any JavaScript event listeners for the close button echo "Checking for close button event listeners in JavaScript:" rg --type js 'getElementById\("close-modal"\)' -A 5 -B 5 # Test 3: Look for any recent changes related to the close button echo "Checking for recent changes related to the close button:" git log -p -S "close-modal" --since="1 month ago"Length of output: 497
app/views/core/ModalView.js (3)
54-54: Approved: Removal of return statement improves method flow.The removal of the return statement from
this.trapFocus()is a good change. It ensures that therendermethod completes its execution after trapping the focus, which is the expected behavior.For improved clarity, consider adding a comment explaining the purpose of calling
trapFocushere:// Ensure keyboard focus is trapped within the modal this.trapFocus()
86-93: Approved: Improved error handling and code readability.The changes in the
trapFocusmethod are well-implemented:
- Simplified condition for checking
this.focusTrap.- Improved error handling with optional chaining.
- Updated console logging with optional chaining.
These changes enhance code readability and reduce the risk of null pointer exceptions.
For consistency, consider using optional chaining for the
console.logstatement in the catch block as well:console.log(this.constructor?.name, 'not trapping focus for modal with no focusable elements')
Line range hint
1-150: Overall: Good improvements to modal functionality and code quality.The changes in this file contribute to better code maintainability, reduced risk of runtime errors, and improved accessibility through enhanced focus trapping. These modifications align well with the PR objectives of enhancing the modal functionality and user experience.
Consider adding JSDoc comments to the
ModalViewclass and its methods to improve code documentation and maintainability. This will be particularly helpful for thetrapFocusmethod, which plays a crucial role in accessibility.app/styles/play/level/modal/hero-victory-modal.sass (2)
100-104: LGTM: Close button styling added.The close button styling has been appropriately positioned. However, consider the following suggestions:
- Add a specific color to ensure visibility across different themes.
- Consider adding hover and focus states for better user interaction.
- Ensure the large font size (50px) doesn't cause any layout issues.
Would you like me to provide example styles for hover and focus states?
Line range hint
365-377: LGTM: Reward pulse animation added.The
rewardPulsekeyframes and corresponding animations for XP and gems look good. They will create an engaging visual effect for the rewards.Suggestion for optimization:
Consider using thewill-changeproperty to hint the browser about the animation:.xp .pulse, .gems .pulse will-change: transformThis can potentially improve performance, especially on mobile devices.
Would you like me to provide more information about the
will-changeproperty and its benefits?app/views/play/level/modal/HeroVictoryModal.js (1)
330-330: Inconsistent Use of Optional Chaining Operator?.The optional chaining operator
?.is used here but isn't consistently used elsewhere in the codebase. For consistency and readability, consider using standard jQuery chaining or consistently using optional chaining throughout the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- app/styles/play/level/modal/course-victory-modal.sass (1 hunks)
- app/styles/play/level/modal/hero-victory-modal.sass (1 hunks)
- app/templates/play/level/modal/hero-victory-modal.pug (1 hunks)
- app/views/core/ModalView.js (2 hunks)
- app/views/play/level/modal/HeroVictoryModal.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
app/styles/play/level/modal/course-victory-modal.sass (1)
21-21: LGTM: Box shadow removal addresses the visual glitch.This change aligns with the PR objectives by removing the extra box shadow from the CourseVictoryModal. It should resolve the visual glitch mentioned in the PR summary, improving the overall appearance of the modal.
app/templates/play/level/modal/hero-victory-modal.pug (1)
Line range hint
1-238: LGTM: Close button addition improves user experienceThe addition of the close button to the hero victory modal aligns well with the PR objectives to enhance user experience. This change allows users to easily dismiss the modal, providing an alternative to the existing "Continue" button.
The rest of the file remains unchanged, preserving the existing functionality of the victory modal, including:
- Display of victory text and course information
- Level feedback and rating system
- Achievement and reward displays
- XP and gem totals
- Sharing options
- Navigation buttons for continuing or returning to other sections
These preserved elements ensure that the modal continues to provide valuable information and options to the user upon completing a level.
app/styles/play/level/modal/hero-victory-modal.sass (1)
Line range hint
1-377: Summary: Changes align with PR objectives and enhance user experience.The modifications to this SASS file successfully implement the close button for the victory modal and improve the visual feedback for rewards. These changes align well with the PR objectives and should enhance the overall user experience.
Key points:
- The close button styling allows for easy modal dismissal.
- The reward pulse animations add engaging visual feedback.
These changes contribute to addressing the usability concern mentioned in the linked issue CCJ-123, providing users with more control over the victory modal interaction.
| button.close-button.btn.btn-illustrated.btn-lg.btn-warning(type="button", data-dismiss="modal", aria-hidden="true", tabindex=-1) × | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider relocating the close button to the modal header
The close button has been added to the modal body content, which is an unusual placement. Typically, close buttons are located in the modal header for consistency and user expectation.
- Consider moving this button to the
modal-header-contentblock for better alignment with standard modal designs. - The
tabindex="-1"attribute may interfere with keyboard navigation. Consider removing it to allow natural tab order, or ensure it doesn't negatively impact accessibility. - For better accessibility, consider adding an aria-label to the button, e.g.,
aria-label="Close".
Here's a suggested relocation of the close button to the modal header:
block modal-header-content
button.close-button.btn.btn-illustrated.btn-lg.btn-warning(type="button", data-dismiss="modal", aria-hidden="true", aria-label="Close") ×
#victory-header.out
// ... rest of the header contentThis change would place the close button in a more standard location and improve its accessibility.
Also makes it so that keyboard focus goes to "continue" button in victory modal, which lets you press escape to close the modal, instead of staying in the ace editor.
Fixes a random extra box shadow in CourseVictoryModal that was causing minor visual glitching.
Trivial refactor to some logging code that was decaffeinated poorly and logging in two cases instead of one.
Summary by CodeRabbit
New Features
Style Updates
Bug Fixes