Vertical global map for Catalyst #7950
Conversation
WalkthroughThis pull request modifies the Catalyst experiment feature within the CodeCombat codebase. The Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (10)
✨ 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/models/User.js (1)
1318-1360: The Catalyst experiment implementation follows established patterns.The
getCatalystExperimentValue()method follows the same pattern as other experiment methods in the class, providing a consistent approach to feature experimentation. The method:
- Checks query parameters first
- Falls back to stored experiment values
- Applies appropriate exclusion criteria (Ozaria users, library networks, China users, classroom users, premium users)
- Uses server-configured probability for assignment
- Properly logs and starts the experiment
Consider adding a brief code comment at the top of the method explaining what the "Catalyst" experiment is testing, similar to the comment on line 1318.
// "Catalyst" is a new global map and UI/UX for CodeCombat Home version getCatalystExperimentValue () { + // This experiment tests a new vertical layout for the campaign map + // to improve navigation and visibility of available content let value = { true: 'beta', false: 'control', control: 'control', beta: 'beta' }[utils.getQueryVariable('catalyst')] if (value == null) { value = me.getExperimentValue('catalyst', null, 'beta') } // Don't include Ozaria for now if ((value == null) && utils.isOzaria) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/models/User.js(1 hunks)app/styles/play/campaign-view.sass(7 hunks)app/templates/play/campaign-view.pug(1 hunks)app/views/play/CampaignView.js(4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
app/models/User.js (6)
app/core/utils.js (2)
value(82-82)value(102-102)app/core/application.js (6)
value(242-242)utils(62-62)utils(164-164)utils(214-214)utils(224-224)userUtils(63-63)app/core/vueRouter.js (1)
utils(3-3)app/core/store/modules/me.js (1)
utils(5-5)app/views/play/CampaignView.js (2)
utils(19-19)userUtils(49-49)app/views/core/CreateAccountModal/BasicInfoView.js (1)
userUtils(29-29)
🔇 Additional comments (10)
app/templates/play/campaign-view.pug (1)
293-419: Well-structured vertical layout implementation for the Catalyst experiment.The implementation of the vertical campaign layout looks well-organized, creating a clear hierarchy with:
- A top row for "junior" and "dungeon" campaigns
- Main campaign rows with side development tracks (game dev on left, web dev on right)
The template correctly handles different campaign states (locked/unlocked), preserves age-appropriate information, and maintains consistent UI patterns across campaign types.
app/views/play/CampaignView.js (5)
111-113: Good addition of event handlers for the new Catalyst portal layout.Adding event listeners for the new portal elements ensures that clicks on the new campaign elements are properly handled by the existing
onClickPortalCampaignmethod.
153-158: Well-implemented feature flag and experiment handling.The code properly handles the 'catalyst' terrain parameter in the URL and sets the
isCatalystproperty based on the user's experiment value. This ensures the feature is only available to users in the 'beta' experiment group.
1320-1327: Good control flow for mouse events in Catalyst view.The conditional returns in the mouse event handlers prevent the default portal scrolling behavior when in Catalyst view, which is appropriate since the new vertical layout doesn't require the same horizontal scrolling mechanism.
1334-1334: Improved selector for finding portal elements.The updated selector now finds both standard portals and the new catalyst portals, ensuring event handling works properly across both UI implementations.
1745-1745: Enhanced click handler for all campaign types.The click handler has been updated to include the new campaign types (
main-campaignandside-campaign), which allows the new UI elements to trigger the same navigation functionality as the original design.app/styles/play/campaign-view.sass (4)
7-11: Good use of variables for campaign dimensions.Adding these variables makes the styling more maintainable and ensures consistency across the different elements. The hover scale is also extracted as a variable, which will make future adjustments easier.
803-805: Well-implemented variable usage.Using the newly defined variables (
$campaignPortalHeightand$campaignHoverScale) for calculating the portals height creates more consistent and maintainable styles.
812-815: Consistent dimension handling with variables.Replacing hardcoded values with the new variables (
$campaignPortalWidthand$campaignPortalHeight) improves style consistency and maintainability.
961-1190: Well-structured CSS for new vertical layout.The new
.portal-catalyststyles create a completely different layout paradigm (vertical instead of horizontal) while maintaining the look and feel of the original UI. Key improvements:
- Good use of flexbox for responsive positioning
- Proper z-indexing for overlap effects
- Consistent hover animations
- Reuse of existing background images with new positioning
- Well-structured class hierarchy for organization
The CSS is thorough and addresses all aspects of the new layout including:
- Main campaign positioning
- Side campaign container positioning
- Proper text styling for labels
- Responsive layout considerations
a581022 to
39316ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/styles/play/campaign-view.sass (1)
961-1189: Well-structured CSS for the new catalyst portal layoutThe new styles for
.portal-catalystare well-organized with a clear hierarchy. The flexbox layout provides a modern and responsive design approach.Suggestions for improvement:
- Consider creating more shared styles between the regular portal and catalyst portal to reduce duplication
- Some hardcoded values like
317pxon line 984 should use the$campaignPortalWidthvariable- Consider creating a variable for the magic number
450pxused in side campaign positioning- width: 317px + width: $campaignPortalWidthAnd for the side campaign positioning:
- left: calc(50% - 450px) + left: calc(50% - $sideCampaignOffset) - right: calc(50% - 450px) + right: calc(50% - $sideCampaignOffset)Where
$sideCampaignOffsetwould be a new variable defined at the top of the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/models/User.js(1 hunks)app/styles/play/campaign-view.sass(7 hunks)app/templates/play/campaign-view.pug(1 hunks)app/views/play/CampaignView.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/models/User.js
- app/views/play/CampaignView.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node.js CI (18.10.0)
🔇 Additional comments (5)
app/templates/play/campaign-view.pug (1)
293-419: New catalyst portal layout implementation looks good!The implementation creates a vertical layout for campaigns with a structured organization - top row for junior/dungeon campaigns and main section for additional campaigns with side campaigns for game/web development. The conditional rendering based on
isCatalystis well implemented.A few suggestions for further improvement:
- Consider extracting the campaign rendering logic into reusable mixins to reduce duplication between this and the default portal view
- The campaign row structure with hardcoded campaign slugs might make future changes more difficult - consider making this data-driven
app/styles/play/campaign-view.sass (4)
7-11: Good addition of dimension variables for better maintainabilityExtracting these campaign dimensions into variables makes the code more maintainable and consistent throughout the stylesheet.
803-805: Using variables for campaign height calculationGood job using the newly defined variables for calculating the portals height, making the code more maintainable.
812-815: Leveraging variables for campaign dimensionsGood refactoring to use the variable-based approach for campaign dimensions.
835-846: Consistent use of portal width variableGood job updating the background positions to use the variable consistently.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/views/play/CampaignView.js (1)
829-829: Consider refactoring tooltip application to reduce duplication.The updated tooltip code now includes new selectors for the Catalyst layout, but this approach might lead to duplication if more campaign types are added in the future.
- _.defer(() => this.$el?.find('.game-controls .btn:not(.poll), .campaign.locked, .beta-campaign.locked, .side-campaign.locked, .main-campaign.locked').addClass('has-tooltip').tooltip()) // Have to defer or i18n doesn't take effect. + const lockedCampaignTypes = ['.campaign', '.beta-campaign', '.side-campaign', '.main-campaign']; + const tooltipSelector = ['.game-controls .btn:not(.poll)', ...lockedCampaignTypes.map(type => `${type}.locked`)].join(', '); + _.defer(() => this.$el?.find(tooltipSelector).addClass('has-tooltip').tooltip()) // Have to defer or i18n doesn't take effect.app/styles/play/campaign-view.sass (2)
961-1195: Well-structured CSS for the vertical Catalyst layout.The new
.portal-catalystsection provides a comprehensive set of styles for the vertical layout. The flexbox-based design with rows and columns is a good approach for responsive layout.I would recommend adding additional media queries for smaller screens, as some fixed pixel values (like
left: calc(50% - 450px)) might cause layout issues on mobile devices.@media screen and (max-width: 900px) .portal-catalyst .portals .campaign-rows .campaign-row .side-campaign-container &.left-campaign, &.right-campaign position: relative left: auto right: auto top: auto transform: none margin: 20px auto
1120-1125: Fixed positioning might cause responsive layout issues.The hard-coded
left: calc(50% - 450px)andright: calc(50% - 450px)values could cause layout problems on smaller screens. Consider using relative positioning or percentage-based values.- &.left-campaign - left: calc(50% - 450px) - - &.right-campaign - right: calc(50% - 450px) + &.left-campaign + left: calc(50% - 140vw / 3) + @media screen and (max-width: 1200px) + left: 5% + + &.right-campaign + right: calc(50% - 140vw / 3) + @media screen and (max-width: 1200px) + right: 5%
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/styles/play/campaign-view.sass(7 hunks)app/templates/play/campaign-view.pug(1 hunks)app/views/play/CampaignView.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/templates/play/campaign-view.pug
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node.js CI (18.10.0)
🔇 Additional comments (11)
app/views/play/CampaignView.js (7)
153-155: Appropriate handling of catalyst query parameter.Good approach for processing the "catalyst" query parameter by resetting the terrain to an empty string, allowing other code paths to handle the experiment flag.
157-159: Feature flag implementation for Catalyst experiment.The code properly checks for the Catalyst experiment value via
me.getCatalystExperimentValue(). This allows for controlled rollout of the new vertical map layout.
111-113: Added event handlers for new campaign elements.You've properly extended the click event handling to include the new
.portal-catalystelements, ensuring consistent user interaction across different campaign layouts.
1322-1325: Early return for Catalyst view in mouse enter handler.Good implementation to bypass hover effects for the Catalyst view. This prevents undesired scrolling behavior in the vertical layout.
1328-1331: Early return for Catalyst view in mouse leave handler.Consistent with the mouse enter handler, this prevents portal scrolling for the Catalyst layout.
1336-1336: Updated selector for portal elements.The selector now includes both
.portaland.portal-catalystelements, ensuring mouse move events work across both layouts.
1747-1747: Enhanced click handler for campaign elements.The click handler now properly captures
.main-campaignand.side-campaignelements, allowing the new vertical layout to maintain the same navigation functionality.app/styles/play/campaign-view.sass (4)
7-11: Good addition of dimension variables for better maintainability.Defining these campaign dimensions as variables will make future updates more maintainable and consistent across the stylesheet.
804-805: Updated portal heights to use variables.The portal height is now properly calculated using the defined variables, maintaining consistency with the hover scale.
812-814: Campaign element dimensions now using variables.The campaign elements now use the defined dimension variables, making the code more maintainable.
835-845: Updated background positions to use variables.Good update to use the variable
$campaignPortalWidthfor background positioning, which will make future changes to campaign dimensions easier.
There was a problem hiding this comment.
Pull Request Overview
This PR implements a vertical layout update for Catalyst campaigns by modifying the experiment handling and view interactions.
- In User.js, the catalyst experiment logic is updated to handle the case when experiment probability is not defined.
- In CampaignView.js, additional click event bindings and view adjustments have been added for the new catalyst campaign elements.
Reviewed Changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| app/models/User.js | Updated catalyst experiment configuration logic in experiment startup. |
| app/views/play/CampaignView.js | Enhanced event bindings and skipped portal scrolling behavior for catalyst experiments. |
Files not reviewed (2)
- app/styles/play/campaign-view.sass: Language not supported
- app/templates/play/campaign-view.pug: Language not supported
Comments suppressed due to low confidence (1)
app/models/User.js:1349
- If probability is defined and Math.random() is not less than probability, no value is assigned, which may lead to an undefined experiment value. Consider adding an else branch to explicitly set the experiment value to 'control' with a corresponding probability value when the condition fails.
} else if (Math.random() < probability) {
…the experiment is not running, defaulting to 'control' group.
- Introduced new styles for the portal-catalyst, including layout and hover effects. - Updated campaign view template to render campaigns within the portal-catalyst. - Enhanced JavaScript to handle interactions for both main and side campaigns in the portal-catalyst. [DISCLAIMER] ITS NOT FINAL AND MESSY
- Adjusted the positioning of campaign elements to enhance alignment and responsiveness. - Added margin to the main campaign container for better spacing. - Updated left and right positioning calculations for game and web development campaigns.
- Changed class names for side campaign elements to improve semantic meaning. - Introduced a new container for side campaigns to better organize layout. - Updated conditional rendering for campaign descriptions and play buttons for consistency.
- Renamed class for side campaign elements to enhance clarity. - Adjusted positioning and visibility for side campaign containers. - Introduced a new background container for better image handling and responsiveness.
- Adjusted flex properties for campaign rows to enhance alignment and spacing. - Reduced gap between campaign rows and removed bottom margin for better visual consistency. - Added hover transition effect to main campaign elements for improved interactivity.
- Simplified the definition of campaign rows in the template for better readability. - Enhanced the campaign selection logic to include additional campaign classes for improved targeting in the JavaScript functionality.
- Adjusted margin and height properties for campaign elements to improve spacing and alignment. - Introduced dynamic margin calculations based on campaign portal height for better adaptability. - Added z-index to campaign elements for improved stacking context.
- Introduced a new variable for beta image width to streamline background size calculations. - Adjusted dimensions and positioning of campaign elements for better alignment and visual consistency. - Refined hover effects and transitions for campaign elements to enhance interactivity.
- Removed commented-out styles in the SASS file to clean up the code. - Updated the campaign rows definition in the Pug template for improved readability while maintaining existing functionality.
- Updated campaign row definitions in the Pug template to include a 'silhouette' class for campaigns without data, improving visual feedback. - Added hover effects in the SASS file to apply contrast and brightness filters to silhouette campaigns, enhancing user experience.
- Added a new 'locked' class in the SASS file to visually indicate locked campaigns with adjusted contrast and cursor styles. - Updated the Pug template to include a data attribute for internationalization of the locked campaign title, improving accessibility and user experience.
- Refactored SASS styles to replace nth-child selectors with specific classes for left and right campaign containers, enhancing clarity and maintainability. - Updated Pug template to apply new class names for left and right campaigns, improving semantic structure and styling consistency.
- Added conditional rendering for game control buttons based on user features and settings in the Pug template, improving user experience for Catalyst users. - Maintained existing functionality for non-Catalyst users while ensuring clarity and maintainability in the template structure.
- Added a new SASS class for game controls, enhancing layout and styling for Catalyst users. - Updated the Pug template to reflect the new class structure, ensuring proper rendering of game control buttons based on user features. - Improved responsiveness and visual feedback for various button states, enhancing user experience.
…ton features - Introduced a jiggle animation for highlighted buttons in the SASS file, improving visual feedback. - Updated the Pug template to include new button elements for HackStack and Roblox, enhancing user interaction options. - Adjusted styles for button states and layout, ensuring a consistent and responsive design across different screen sizes.
…rience - Updated the Pug template to conditionally render game control buttons for Catalyst users only when a campaign is not present, enhancing clarity in the user interface. - Modified the JavaScript to ensure tooltips are applied to buttons in both standard and Catalyst game controls, improving accessibility and interaction feedback.
- Added a new image asset for the HackStack feature, enhancing visual representation. - Updated SASS styles to change the background image for the HackStack menu icon, improving branding consistency. - Modified JavaScript to ensure tooltips are applied to additional buttons, enhancing user interaction fee 6302 dback.
…gn view - Introduced new image assets for the AI League and Roblox features, enhancing visual representation. - Updated SASS styles to implement new background images and improved transformations for menu icons, ensuring a consistent user experience. - Modified the Pug template to conditionally render the AI League button, expanding user interaction options. - Enhanced JavaScript to track interactions with the AI League menu icon, improving engagement analytics.
1290aad to
8a41050
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/styles/play/campaign-view.sass (3)
609-834: Consider reducing CSS duplication with mixinsThere's significant duplication between the regular
.game-controlsand the new.game-controls-catalystand.other-products-catalyststyles. Consider extracting the common button styles into a SASS mixin or shared class to improve maintainability.For example:
@mixin game-button-styles width: $gameControlSize height: $gameControlSize background-size: cover @include transition(0.5s ease) @include box-shadow(2px 2px 4px black) border: 0 border-radius: 12px filter: none &:hover @include box-shadow(0 0 12px #bbf) &:active, &.highlighted @include box-shadow(0 0 20px white)This would reduce duplication and make future changes easier to implement.
1195-1429: Use existing variables consistently for better maintainabilityThe portal-catalyst styles have some hardcoded dimension values that should use the variables defined at the top of the file:
- Lines 1218-1219: Replace
width: 317pxandheight: 634pxwithwidth: $campaignPortalWidthandheight: $campaignPortalHeight- Lines 1355-1356 and 1359-1360: Consider defining a variable for the
450pxoffset valueUsing variables consistently will make future adjustments easier and ensure visual consistency across different elements.
1417-1428: Convert hardcoded background positions to use variablesThe background positions for side campaigns use hardcoded pixel values (e.g.,
-454px,-628px,-151px) instead of calculated positions based on variables. For better maintainability and consistency, consider using calculations with the defined variables:// Example for game dev campaigns &.campaign-game-dev-1 background-position: ((-4.5 * $betaImagesWidth / 10)) 0px &.campaign-game-dev-2 background-position: ((-6.5 * $betaImagesWidth / 10)) 0px // Example for web dev campaigns &.campaign-web-dev-1 background-position: ((-1.5 * $betaImagesWidth / 10)) 0px &.campaign-web-dev-2 background-position: ((-3 * $betaImagesWidth / 10)) 0pxThis approach would make it easier to adjust image sizes in the future while maintaining proper alignment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
app/assets/images/pages/hackstack/Code-Combat-AI-Hack-Stack-Full-Colour-Small.pngis excluded by!**/*.pngapp/assets/images/pages/league/Code-Combat-AI-League-Small.pngis excluded by!**/*.pngapp/assets/images/pages/roblox/Code-Combat-Worlds-Small.pngis excluded by!**/*.png
📒 Files selected for processing (4)
app/models/User.js(1 hunks)app/styles/play/campaign-view.sass(8 hunks)app/templates/play/campaign-view.pug(2 hunks)app/views/play/CampaignView.js(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/User.js
🔇 Additional comments (14)
app/templates/play/campaign-view.pug (3)
293-485: Well-structured implementation of the vertical campaign layoutThe new catalyst portal view provides an excellent vertical organization of campaigns with a logical grouping:
- Top row for junior and dungeon campaigns
- Main campaigns (forest, desert, mountain, glacier) in the center
- Side campaigns for specialized content (game development, web development)
This implementation aligns well with the PR objective of creating a vertical island layout, and the code maintains a consistent structure for rendering campaigns across different types.
334-340: Excellent data-driven approach for campaign organizationThe campaign rows structure provides a clear data model that separates the layout logic from the content. By defining each row with a main campaign and optional side campaigns, the code becomes more maintainable and easier to extend in the future if more campaigns need to be added.
485-507: Clear separation of control elements improves UI organizationThe controls are now logically separated into two groups in the catalyst view:
- Game-related controls positioned on the left side
- Other product offerings positioned on the right side
This separation follows good UI design principles by grouping related functions together and matches the PR objective of "moving game buttons to the left, positioning other products to the right."
app/styles/play/campaign-view.sass (2)
7-11: Good use of variables for improved maintainabilityIntroducing variables for campaign dimensions (
$campaignPortalWidth,$campaignPortalHeight, etc.) makes the stylesheet more maintainable. This approach centralizes these values, making it easier to adjust dimensions throughout the stylesheet by changing just one value.
1195-1204: Well-implemented vertical layout using modern CSS techniquesThe portal-catalyst styles implement a vertical layout using flexbox, which is the appropriate technology choice for this requirement. The layout is responsive and well-structured, with clear organization between the different campaign sections.
The implementation aligns well with the PR objectives of creating a vertical island layout and reorganizing the UI elements.
app/views/play/CampaignView.js (9)
98-98: Good addition of new event handler for the AI League icon.This handler connects clicks on the AI League menu icon to the corresponding method, maintaining consistency with other icon click handlers.
112-114: Excellent implementation of Catalyst portal campaign selectors.These new event handlers properly route clicks from the Catalyst portal's various campaign types to the existing
onClickPortalCampaignmethod, enabling a consistent user experience across different UI layouts.
154-159: Proper initialization of Catalyst experiment.The code correctly processes the Catalyst query parameter and initializes the
isCatalystproperty based on the experiment value. This allows for conditional rendering and behavior based on whether the user is in the experiment group.
835-835: Good tooltip handling for Catalyst UI elements.The tooltip initialization is properly extended to include the new Catalyst UI elements, ensuring consistent tooltip behavior throughout the application.
1328-1328: Appropriate conditional behavior for portal scrolling.These conditionals properly skip the portal scrolling behavior for users in the Catalyst experiment, which is necessary since the new vertical layout likely doesn't require or support the same scrolling behavior.
Also applies to: 1334-1334
1342-1342: Good portal selector update for mouse movement handling.The selector is properly updated to include both standard portals and Catalyst portals, ensuring mouse interaction works correctly with both UI layouts.
692-695: Well-implemented AI League icon click handler.The implementation tracks user engagement and opens the appropriate promotion modal, following the established pattern for other similar handlers in the file.
1752-1754: Comprehensive update to portal campaign click handling.The selector is now correctly expanded to include all campaign types, including the new Catalyst-specific ones (side-campaign, main-campaign), ensuring clicks are properly handled regardless of UI layout.
2197-2199: Appropriate visibility logic for AI League.This addition ensures the AI League is only shown to appropriate users (not library users or those in editor mode), following the established pattern for feature visibility in the application.
- Adjusted z-index values in the SASS file to ensure proper stacking order of elements in the campaign view. - Standardized z-index values for buttons and overlays, enhancing visual hierarchy and user interaction clarity.
- Introduced a new user status section in the Pug template for Catalyst users, enhancing visibility of player information. - Updated SASS styles to style the user status elements, including player level, gems, and hero icons, improving overall aesthetics. - Ensured conditional rendering of user status based on user type, maintaining clarity for both Catalyst and non-Catalyst users.
- Simplified the display of anonymous player names in the Pug template for Catalyst users, enhancing clarity. - Added new SASS styles for star icons and language buttons, improving visual consistency and user interaction. - Adjusted margins and padding in the campaign view styles to enhance layout and responsiveness.
- Introduced a new utility controls section in the Pug template, enhancing the layout for volume and language selection buttons. - Added SASS styles for utility controls, including button hover and active states, improving user interaction feedback. - Removed deprecated button elements to streamline the template and improve clarity.
- Changed margin-top to a fixed value of 50px and set margin-bottom to -60px for better spacing in the campaign view. - Adjusted margin-top for the campaign element to -30px, enhancing the overall layout and visual consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/templates/play/campaign-view.pug (1)
536-562: Consider DRYing up repeated user status code.There's significant code duplication between the standard user status section and the catalyst-specific section. Consider extracting common functionality into a mixin or helper.
- if view.isCatalyst && !campaign - .user-status-catalyst.header-font.picoctf-hide - .user-status-line - if view.shouldShow('status-line') - span.player-hero-icon - if me.get('anonymous') - span.player-name(data-i18n="play.anonymous") Anonymous - else - if me.isStudent() - a(href="/play").player-name.spr= me.get('name') - else - a(href="/account").player-name.spr= me.get('name') - if view.shouldShow('gems') - span.gem.gem-30 - span#gems-count.spr= me.gems() - if view.shouldShow('level') - .rtl-allowed - span.star.star-30 - span.player-level.spr= me.level() - if me.get('anonymous') - button.btn.btn-illustrated.login-button.btn-warning(data-i18n="login.log_in") - button.btn.btn-illustrated.signup-button.btn-danger(data-i18n="signup.sign_up") - else - - if !me.isStudent() - button#logout-button.btn.btn-illustrated.btn-warning(data-i18n="login.log_out") Log Out - //- if me.isPremium() - //- button.btn.btn-illustrated.btn-primary(data-i18n="nav.contact", data-toggle="coco-modal", data-target="core/ContactModal") Contact - if me.getM7ExperimentValue() == 'beta' - span.beta-levels - span(data-i18n="play.beta_levels_on") - | - span.m7-off(data-i18n="play.beta_levels_turn_off") + // Create a mixin for user status that can be reused + mixin userStatus(containerClass) + div(class=containerClass + " header-font picoctf-hide") + .user-status-line + if view.shouldShow('status-line') + span.player-hero-icon + if me.get('anonymous') + span.player-name(data-i18n="play.anonymous") Anonymous#{containerClass === "user-status-catalyst" ? "" : " Player"} + else + if me.isStudent() + a(href="/play").player-name.spr= me.get('name') + else + a(href="/account").player-name.spr= me.get('name') + if view.shouldShow('gems') + span.gem.gem-30 + span#gems-count.spr= me.gems() + if view.shouldShow('level') + .rtl-allowed + if containerClass === "user-status-catalyst" + span.star.star-30 + else + span.level-indicator(data-i18n="general.player_level") + span.player-level.spr= me.level() + if me.get('anonymous') + button.btn.btn-illustrated.login-button.btn-warning(data-i18n="login.log_in") + button.btn.btn-illustrated.signup-button.btn-danger(data-i18n="signup.sign_up") + else + if me.isStudent() + a(href="/play").player-name.spr= me.get('name') + else + a(href="/account").player-name.spr= me.get('name') + if !me.isStudent() + button#logout-button.btn.btn-illustrated.btn-warning(data-i18n="login.log_out") Log Out + if containerClass !== "user-status-catalyst" + select.language-dropdown.btn.btn-illustrated.btn-primary + if me.getM7ExperimentValue() == 'beta' + span.beta-levels + span(data-i18n="play.beta_levels_on") + | + span.m7-off(data-i18n="play.beta_levels_turn_off") + if view.isCatalyst && !campaign + +userStatus("user-status-catalyst")app/styles/play/campaign-view.sass (1)
1676-1725: Consider adding responsive design for smaller screens.The utility controls and language dropdown styles could benefit from responsive adjustments for smaller screen sizes, similar to how you've handled the game controls with media queries.
.utility-controls-catalyst position: absolute left: 20px bottom: 20px z-index: 10 display: flex gap: 10px + + @media only screen and (max-height: 650px) + left: 10px + bottom: 10px + gap: 5px .btn width: 40px height: 40px padding: 3px 8px @include opacity(0.75) @include box-shadow(2px 2px 4px black) border: 0 border-radius: 12px + + @media only screen and (max-height: 650px) + width: 30px + height: 30px + padding: 2px 6px &:hover @include opacity(1.0) @include box-shadow(0 0 12px #bbf) &:active, &.highlighted @include box-shadow(0 0 20px white) .glyphicon font-size: 28px + + @media only screen and (max-height: 650px) + font-size: 20px #volume-button margin-right: 10px &.language-dropdown width: 112px + + @media only screen and (max-height: 650px) + width: 80px
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/locale/en.js(1 hunks)app/styles/common/common.sass(1 hunks)app/styles/play/campaign-view.sass(12 hunks)app/templates/play/campaign-view.pug(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node.js CI (18.10.0)
🔇 Additional comments (9)
app/locale/en.js (1)
570-570: Simplified anonymous user label.The label for anonymous users has been shortened from "Anonymous Player" to "Anonymous", making the UI more concise. This change aligns with the PR objectives regarding UI standardization and clarity.
app/styles/common/common.sass (1)
226-248: Excellent addition of the star classes.The new
.starclass and its size variants are well-structured and follow the same pattern as the existing.gemclass. This provides consistent styling options for star elements throughout the application.app/templates/play/campaign-view.pug (3)
293-420: Well-structured implementation of the vertical campaign layout.The new catalyst portal layout is well organized with a clear separation between the top row (junior and dungeon campaigns) and the main campaign rows. The code efficiently reuses campaign data and follows consistent patterns for displaying campaign information, locked states, and play buttons.
485-563: Good separation of game controls for the catalyst view.The game control buttons are nicely organized with left-side placement for main game features and right-side placement for other products, aligning with the PR objectives. The implementation maintains all functionality while improving the UI organization.
608-612: Use the star class for level indicator in Catalyst view.Great job using the newly added star class for level indication in the Catalyst view. This maintains visual consistency with other star elements in the application.
app/styles/play/campaign-view.sass (4)
7-11: Good addition of variables for campaign dimensions.Adding these variables makes the CSS more maintainable and consistent. If campaign dimensions need to be changed in the future, they can be adjusted in a single place.
609-716: Well-structured styles for game controls in Catalyst view.The game-controls-catalyst styles are well-organized and maintain consistency with the existing game controls while adapting to the new left-side positioning specified in the PR objectives.
717-834: Good implementation of other-products section.The styling for the other-products-catalyst section supports the PR's objective of positioning other products to the right. The icons are well-sized and positioned with appropriate hover effects.
1300-1535: Well-structured portal-catalyst styles.The portal-catalyst section is well-organized with clear structure for the different campaign types and their layout. The styles maintain consistency with existing patterns while supporting the new vertical layout.
There was a problem hiding this comment.
Great work on this one!!!
A few minor things that can be improved:
- Icons and text appear up and down, should be even in a line
- The "Join Class" modal appears on top of AI Hackstack on my screen. This doesn't happen on all screen sizes and is actually non-blocking for this PR, but some thing to keep in mind.
Yes, they were like that before we just increased their size and now it noticibale. I added an issue for the polishing this |
We are going to remove this pop up at all |
- Added checks to ensure campaigns and sessions are loaded before initiating portal mouse events, improving stability. - Implemented a condition to prevent clearing the portal scroll interval if it is not set, enhancing performance and preventing errors.
Last version
Closing https://linear.app/codecombat/issue/GD-430/vertical-island-layout
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Style
Bug Fixes
Content Updates
Closes GD-455 , GD-456 , GD-565
Standardized UI Panels & Clear Icon Grouping Task