Yuqiang/new modals #7827
Yuqiang/new modals #7827
Conversation
WalkthroughThe pull request introduces significant updates to localization strings across various sections of the application, including the addition, modification, and removal of entries in Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using 8000 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/templates/core/ai-league-promotion-modal.pug (1)
Line range hint
6-7: Add accessibility attributes to close buttonThe close button needs proper accessibility attributes for screen readers.
Apply these changes:
- div.close-modal - span.glyphicon.glyphicon-remove + div.close-modal(role="button" aria-label="Close modal" tabindex="0") + span.glyphicon.glyphicon-remove(aria-hidden="true")app/templates/core/junior-modal.pug (2)
11-13: Add descriptive alt text for accessibilityWhile the image path and i18n implementations are correct, the empty alt attribute should include descriptive text for screen readers.
- img.hero-image.junior(src="/images/pages/junior/codecombat-junior.png" alt="") + img.hero-image.junior(src="/images/pages/junior/codecombat-junior.png" alt="CodeCombat Junior hero image")
Line range hint
1-24: Well-structured modal implementationThe modal's structure effectively separates the junior and regular CodeCombat sections while maintaining clean markup and proper internationalization. The layout provides a clear choice for users, aligning well with the "Choose Your Adventure" concept.
Consider adding data-testid attributes to key elements to facilitate future testing and maintenance.
app/styles/modal/ai-league-promotion-modal.sass (2)
68-71: LGTM! Consider adding responsive sizing constraints.The centered iframe styling looks good. However, for better responsive behavior, consider adding max-width and min-width constraints.
iframe margin-left: auto margin-right: auto + max-width: 100% + min-width: 300px
Line range hint
6-8: Consider implementing a responsive layout.The modal uses fixed dimensions (746x520px) which might not work well on smaller screens. Consider using relative units and media queries for better mobile support.
Example approach:
.modal-dialog - width: 746px - height: 520px + width: min(746px, 90vw) + height: min(520px, 80vh) + @media (max-width: 768px) + margin: 20px auto 0 autoapp/templates/core/subscribe-modal.pug (2)
40-40: Consider adding fallback for .webp image format.While .webp provides better performance, some older browsers might not support it. Consider adding a fallback image:
- img(src="/images/pages/home/anya-and-nando-pet.webp") + picture + source(srcset="/images/pages/home/anya-and-nando-pet.webp" type="image/webp") + img(src="/images/pages/home/anya-and-nando-pet.png" alt="Premium features illustration")
Line range hint
1-124: Consider accessibility improvements.The modal structure is good, but could benefit from these accessibility enhancements:
- Add ARIA labels for error states:
- #declined-alert.alert.alert-danger.alert-dismissible + #declined-alert.alert.alert-danger.alert-dismissible(role="alert" aria-live="polite")
- Replace Unicode lock emoji with an SVG icon or Font Awesome for consistent rendering:
- span.buy-lock-icon 🔒 + span.buy-lock-icon + i.fa.fa-lock(aria-hidden="true")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
app/assets/images/pages/home/anya-and-nando-pet.pngis excluded by!**/*.pngapp/assets/images/pages/junior/codecombat-home.pngis excluded by!**/*.pngapp/assets/images/pages/junior/codecombat-junior.pngis excluded by!**/*.png
📒 Files selected for processing (7)
app/locale/en.js(4 hunks)app/styles/modal/ai-league-promotion-modal.sass(2 hunks)app/styles/modal/junior-modal.sass(1 hunks)app/styles/modal/subscribe-modal.sass(2 hunks)app/templates/core/ai-league-promotion-modal.pug(1 hunks)app/templates/core/junior-modal.pug(1 hunks)app/templates/core/subscribe-modal.pug(1 hunks)
🔇 Additional comments (14)
app/templates/core/ai-league-promotion-modal.pug (1)
Line range hint 1-24: LGTM! Modal structure is well-organized
The modal template is well-structured with:
- Proper localization support through data-i18n attributes
- Responsive layout using Bootstrap grid
- Clear content organization
app/templates/core/junior-modal.pug (2)
4-4: LGTM: Proper internationalization implementation
The change correctly implements i18n support for the modal title using the appropriate Pug syntax for unescaped interpolation.
15-15: Add descriptive alt text and verify image assets
The empty alt attribute should be updated for accessibility. Also, let's verify that the new image assets exist in the repository.
- img.hero-image.codecombat(src="/images/pages/junior/codecombat-home.png" alt="")
+ img.hero-image.codecombat(src="/images/pages/junior/codecombat-home.png" alt="CodeCombat home hero image")✅ Verification successful
Add descriptive alt text for better accessibility
The image assets exist at the correct location (app/assets/images/pages/junior/), but the alt text should still be updated for accessibility purposes.
- Update alt text in
app/templates/core/junior-modal.pug:
- img.hero-image.codecombat(src="/images/pages/junior/codecombat-home.png" alt="")
+ img.hero-image.codecombat(src="/images/pages/junior/codecombat-home.png" alt="CodeCombat home hero image")🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the new image assets exist in the repository
# Expected: Both image files should be present
fd -t f "codecombat-junior.png|codecombat-home.png" images/pages/junior/
Length of output: 182
Script:
#!/bin/bash
# Let's first check if the images directory exists and its structure
fd -t d "images"
# Then search for the image files in the entire repository
fd -t f "codecombat-junior.png|codecombat-home.png"
# Also search with ripgrep to find any references to these images
rg -g "!{node_modules,dist,build}/*" "codecombat-junior\.png|codecombat-home\.png"
Length of output: 574
app/styles/modal/ai-league-promotion-modal.sass (2)
75-75: Verify text readability at 14px font size.
The reduced font size for .med-text might affect readability, especially for users with visual impairments. Consider keeping it at 16px or implementing a minimum font size.
#!/bin/bash
# Check if there are any accessibility guidelines or constants defined in the codebase
rg -g '*.sass' -g '*.scss' -A 3 'font-size.*16px|accessibility|a11y'Also applies to: 78-78
90-90: LGTM! Verify consistency with other action buttons.
The added padding improves the button's usability. Let's verify this matches other play buttons in the codebase.
✅ Verification successful
Let me try a more focused search to find button padding patterns.
Let me search specifically for play button styles to verify consistency.
Padding is consistent with other play buttons in the codebase
The 10px horizontal padding matches other play button implementations, particularly in:
- Campaign view (
app/styles/play/campaign-view.sass) - Level playback controls (
app/styles/play/level/level-playback-view.sass) - Other modal play buttons (
app/styles/modal/junior-modal.sass)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent padding across similar buttons
rg -g '*.sass' -g '*.scss' 'play-button.*padding|button.*success.*padding'
Length of output: 19294
Script:
#!/bin/bash
# Search for button padding patterns in sass/scss files
rg -g '*.sass' -g '*.scss' 'button.*padding|\.btn.*padding' -A 2
Length of output: 1689
Script:
#!/bin/bash
# Search for play button related styles
rg -g '*.sass' -g '*.scss' 'play-button|\.play.*button' -B 2 -A 2
Length of output: 5480
app/styles/modal/junior-modal.sass (1)
157-159: LGTM! Consider verifying responsive behavior.
The image size and position adjustments align with the PR objectives to update the modal's visual presentation. The changes make both hero images more prominent by increasing their width to 60% and adjusting their positioning accordingly.
Since these changes significantly increase the image sizes, please verify the modal's appearance across different screen sizes:
Also applies to: 162-164
✅ Verification successful
Let me gather more information about the responsive behavior and media queries in the junior modal styles.
The image size adjustments are safe with existing responsive behavior
The modal already implements robust responsive handling:
- Scales content based on viewport width below 746px using CSS custom properties
- Hides hero images completely on shorter screens (height < 520px)
- Uses Bootstrap's grid system (
col-xs-6) for layout structure
The increased image sizes (60%) are safe because:
- The modal scales proportionally on smaller screens
- Images are contained within grid columns that maintain relative spacing
- Images are hidden on very small heights where they might cause issues
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all files that might be affected by these modal changes
# Look for related test files or responsive layout tests
# Search for related test files
echo "=== Related test files ==="
fd -e spec.coffee -e spec.js . | rg -i "junior.*modal|modal.*test"
# Search for related template files
echo -e "\n=== Related template files ==="
fd -e pug . | rg -i "junior.*modal"
# Search for related style imports
echo -e "\n=== Style dependencies ==="
rg -l "junior-modal" -g "*.sass" -g "*.scss"
Length of output: 431
Script:
#!/bin/bash
# Check the media queries and responsive styles in junior-modal.sass
echo "=== Media queries in junior-modal.sass ==="
rg "@media" -A 5 -B 2 app/styles/modal/junior-modal.sass
# Check the template file for any responsive classes or container elements
echo -e "\n=== Modal template structure ==="
cat app/templates/core/junior-modal.pug
# Look for any responsive utility classes or mixins that might be used
echo -e "\n=== Responsive utilities ==="
rg "responsive|mobile|tablet|desktop" app/styles/modal/junior-modal.sass
Length of output: 1804
app/templates/core/subscribe-modal.pug (2)
31-31: LGTM! Title change aligns with objectives.
The new title "subscribe.subscribe_to_premium" is more specific and better emphasizes the premium nature of the subscription, which aligns well with the PR objectives.
43-47: Verify new localization strings are present.
The features list has been updated with new strings. Let's verify these strings exist in the localization files:
✅ Verification successful
All localization strings are present and properly formatted
All the required localization strings for the updated features list are present in app/locale/en.js:
feature_productsfeature_heroesfeature_learnfeature_gems(with correct interpolation format forgemsandgems12)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new localization strings exist in en.js
# Search for the new feature strings
echo "Checking for new feature strings..."
rg -A 1 'feature_products|feature_heroes|feature_learn|feature_ai_level_bot' app/locale/en.js
# Verify the string format for gems interpolation
echo "Checking gems string format..."
rg 'feature_gems.*gems.*gems12' app/locale/en.js
Length of output: 826
app/styles/modal/subscribe-modal.sass (2)
109-116: LGTM: Clean implementation of list styling.
The list styling changes improve readability with proper spacing and consistent bullet points. The implementation follows SASS best practices with proper nesting and maintainable custom bullet point styling.
132-134: Verify positioning context for absolutely positioned images.
The use of position: absolute requires a proper positioning context. Ensure that .option-col or a parent element has position: relative set to prevent potential layout issues.
Let's verify the positioning context:
✅ Verification successful
Positioning context is properly set up
The absolutely positioned image is within .option-col which is inside .paper-area. The .paper-area element has position: absolute set, which establishes a positioning context for its descendants. This means the absolute positioning of the image will work as expected, being positioned relative to .paper-area.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any relative positioning on .option-col or its parents
rg -U "\.option-col[^{]*\{[^}]*position:\s*relative" app/styles/
rg -U "\.paper-area[^{]*\{[^}]*position:\s*relative" app/styles/
Length of output: 133
Script:
#!/bin/bash
# Let's check the full context of the subscribe-modal.sass file to understand the structure
cat app/styles/modal/subscribe-modal.sass
# Also search for any relative positioning in parent elements within modal styles
rg -U "position:\s*relative" app/styles/modal/
Length of output: 7130
app/locale/en.js (4)
1415-1417: Verify the new AI feature is integrated correctly.
A new feature feature_ai_level_bot and its description feature_ai_level_bot_desc were added related to AI-powered support.
Ensure the product functionality matches this description and the feature is properly gated behind the paywall.
Manually test the AI-powered support feature and its access control based on subscription status.
4948-4950: Verify the new AI League string is used correctly.
The new string codecombat_ai_league was added.
Verify it is being displayed in the intended place and not conflicting with any existing UI elements.
Manually check the pages where this string is used to ensure it fits well in the layout.
1391-1399: Verify the copy and pricing changes.
The subscribe_to_premium section had a few copy and pricing changes:
- New feature bullet point added for
feature_all_products - Existing feature
feature_productsmodified - Pricing details like
month_price,first_month_price,lifetime_price,year_priceupdated
Double check that these changes are intentional and the new prices are correct. Also verify these strings are not used elsewhere that may need updating.
Run the following script to verify the usage of these strings:
✅ Verification successful
Let me analyze the findings from the search results:
- The review comment is concerned about copy and pricing changes in the
subscribe_to_premiumsection inapp/locale/en.js, specifically:
- New feature bullet point
feature_all_products - Modified feature
feature_products - Updated pricing strings like
month_price,first_month_price,lifetime_price,year_price
-
The search results show that these pricing strings are used consistently across language files for localization, but they are template strings that use a
__price__placeholder. The actual price values are injected at runtime. -
The feature strings
feature_all_productsandfeature_productsare new/modified marketing copy that describe the product offerings. -
The changes appear to be intentional updates to reflect new product bundling and pricing structure, rather than accidental modifications.
-
The strings are used in a standardized way across the localization system and don't have direct code dependencies that would require updates elsewhere.
Based on this analysis, I can provide a final response:
Copy and pricing changes look valid and properly isolated to localization
The changes to the premium subscription copy and pricing strings follow the established localization patterns:
- New/modified feature strings use standard i18n format
- Pricing strings maintain the
__price__template pattern used across all languages - Changes are properly scoped to localization files and don't require code updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify pricing strings are not used elsewhere.
# Check for other usages of the pricing strings
rg -w '(month_price|first_month_price|lifetime_price|year_price)'
Length of output: 581858
598-599: Verify the function signature change in the codebase.
The new string choose_your_adventure was added to the play section. Ensure that all usages of the play object have been updated to handle this new property.
Run the following script to verify the usage:
| @@ -10,7 +10,7 @@ | |||
| div.paper-area | |||
| div.big-text.text-center(data-i18n="league.promotion_blurb") | |||
|
|
|||
| iframe(src="https://www.youtube.com/embed/dM3fUjqLpnI", frameborder="0", allowfullscreen="true", width="100%", height="250px") | |||
| iframe(src="https://www.youtube.com/embed/dM3fUjqLpnI", frameborder="0", allowfullscreen="true", width="70%", height="250px") | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance iframe accessibility and security
The iframe needs improvements for accessibility and security:
Apply these changes:
- iframe(src="https://www.youtube.com/embed/dM3fUjqLpnI", frameborder="0", allowfullscreen="true", width="70%", height="250px")
+ iframe(
+ title="CodeCombat AI League Introduction Video"
+ src="https://www.youtube.com/embed/dM3fUjqLpnI"
+ loading="lazy"
+ sandbox="allow-scripts allow-same-origin allow-presentation"
+ width="70%"
+ height="250px"
+ allowfullscreen
+ )📝 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.
| iframe(src="https://www.youtube.com/embed/dM3fUjqLpnI", frameborder="0", allowfullscreen="true", width="70%", height="250px") | |
| iframe( | |
| title="CodeCombat AI League Introduction Video" | |
| src="https://www.youtube.com/embed/dM3fUjqLpnI" | |
| loading="lazy" | |
| sandbox="allow-scripts allow-same-origin allow-presentation" | |
| width="70%" | |
| height="250px" | |
| allowfullscreen | |
| ) |
|
ok the png file is a mistake, i'll remove it |
fix ENG-1391
update 3 modals:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Style Changes