765-fix: Fix styles active menu item #814
Conversation
|
The arrow's focus state could be improved This isn't due to your changes, but the spacing is inconsistent here |
|
Lighthouse Report:
|
| setDropdownOpen((prev) => !prev); | ||
| } | ||
| }; | ||
|
|
||
| const handleEscKeyPress = (e: KeyboardEvent<HTMLElement>) => { | ||
| if (e.code === 'Escape') { |
There was a problem hiding this comment.
Let's all keys' codes move to constant like KEY_CODES = {ESCAPE: 'Escape', .....} You could add it here src/core/const/index.ts
|
Lighthouse Report:
|
…ip and courses pages
|
@JuliaAnt, please check the keyboard navigation. It seems like this functionality is no longer working |
src/widgets/header/header.tsx
Outdated
| import classNames from 'classnames/bind'; | ||
| import { usePathname } from 'next/navigation'; | ||
|
|
||
| import { BurgerMenu } from './ui/burger/burger'; | ||
| import { donateOptionsForNavMenu } from '../../../dev-data/donate-options.data'; |
There was a problem hiding this comment.
Please add donateOptionsForNavMenu to file dev-data/index.ts
And update import:
import { donateOptionsForNavMenu } from 'data';
| opacity: 0; | ||
| } | ||
|
|
||
| &.visible { | ||
| @extend %transition-all; | ||
|
|
||
| max-height: 624px; | ||
| margin-top: 26px; | ||
| visibility: visible; | ||
| opacity: 1; |
There was a problem hiding this comment.
please use constant for opacity
$opacity-100, $opacity-0
|
Lighthouse Report:
|
|
Lighthouse Report:
|
|
Lighthouse Report:
|
|
Lighthouse Report:
|
|
Noticed some issue with the new ai course |
📝 WalkthroughWalkthroughThis update refactors navigation menus and related UI components. It introduces new menu layouts, improves accessibility and keyboard interaction, adds new color variables and mixins, updates donation data structures, and enhances test coverage. Several components receive new props or styling, and menu logic shifts to a click-to-open model with improved active state visuals. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NavItem
participant DropdownWrapper
participant useOutsideClick
participant Router
User->>NavItem: Clicks menu button
NavItem->>NavItem: Toggle dropdown state
NavItem->>DropdownWrapper: Render dropdown (isOpen)
NavItem->>useOutsideClick: Listen for outside clicks
useOutsideClick-->>NavItem: On outside click, close dropdown
User->>NavItem: Presses Enter/Space/Escape
NavItem->>NavItem: Handle keyboard navigation
NavItem->>Router: Navigate if not dropdown
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/widgets/header/header.module.scss (1)
91-100: 💡 Verification agent🧩 Analysis chain
Verify mobile menu opacity transition consistency.
The opacity transitions should align with the mobile menu open/close behavior. Ensure the opacity values work correctly with the transform animations.
🏁 Script executed:
#!/bin/bash # Description: Check mobile menu interaction patterns and transition consistency # Expected: Find mobile menu state management and transition usage rg -A 5 -B 5 'mobile.*menu.*open\|\.open.*mobile' src/widgets/ ast-grep --pattern 'className={$$$mobile$$$}'Length of output: 112
🏁 Script executed:
#!/bin/bash # Show SCSS context around .mobile-menu rules rg -n -C5 '\.mobile-menu' src/widgets/header/header.module.scss # Find where the mobile-menu class is toggled in the React component rg -n -C3 'mobile-menu' src/widgets/headerLength of output: 3036
Fix mobile menu display and opacity transitions
The
.mobile-menu.openrule currently setsdisplay: none, which prevents any transform or opacity animation from ever showing. To allow a smooth reveal:• In
src/widgets/header/header.module.scss(around lines 70–100):
– Remove or overridedisplay: nonein the open state (e.g. usedisplay: block).
– Ensure the closed state hasopacity: 0and the open state hasopacity: 1.
– Add an opacity transition alongside the transform.Suggested update:
.mobile-menu { - transform: translateY(-100%); + transform: translateY(-100%); + opacity: 0; + display: none; + transition: transform 0.4s ease-out, opacity 0.4s ease-out; } .mobile-menu.open { - transform: translateY(0); - display: none; - opacity: 1; + transform: translateY(0); + opacity: 1; + display: block; }
♻️ Duplicate comments (9)
src/widgets/mobile-view/ui/mobile-nav-item/mobile-nav-item.tsx (1)
29-35: Fix hardcoded alt text for dynamic icon usage.The alt text is hardcoded as "Donate-icon" but should be dynamic based on the icon being displayed, as previously suggested.
Apply this fix to make the alt text contextual:
- alt="Donate-icon" + alt={`${title} icon`}src/widgets/mobile-view/ui/mobile-view.module.scss (2)
4-12: Remove default flex value and consider past feedback.The
align-items: flex-endshould be sufficient here without explicitly setting the defaultflex-startbehavior mentioned in past comments.
47-57: Use color constants for border colors.The divider implementations use hard-coded color variables that should leverage existing constants as suggested in past feedback.
src/widgets/header/header.tsx (3)
8-8: Import path needs updateThe import path for
donateOptionsshould be from the data barrel export.
49-49: Previous suggestion addressedThe removal of dynamic class names addresses the earlier suggestion about removing the white-class when only one color is used.
155-155: Consider component decompositionThe Header component has grown significantly with the addition of the Support Us menu. Consider splitting each NavItem into separate components as previously suggested.
src/widgets/mobile-view/ui/mobile-view.tsx (2)
10-10: Import path needs updateThe import path for
donateOptionsshould be from the data barrel export.
242-242: Component has grown significantlyAs suggested previously, consider splitting this large component into smaller, more manageable pieces. Each menu section could be its own component.
src/widgets/header/ui/nav-item/nav-item.module.scss (1)
115-115: Consider using color constantThe hover color
$color-gray-450should be defined as a constant as previously suggested.
🧹 Nitpick comments (10)
src/shared/ui/logo/logo.module.scss (2)
3-4: Consistent logo sizing across breakpoints
Default logo dimensions are now 48px but identical values are repeated in themedia‐tablet‐largeblock. Consider extracting these into a variable or removing the redundant declarations to adhere to DRY principles.
12-13: Redundant declarations
The width and height here mirror the default styles. Unless future breakpoints require different sizing, you can safely remove these rules.src/shared/hooks/use-outside-click/use-outside-click.ts (1)
3-7: Consider using useCallback for the callback parameter.The hook works correctly, but including the callback in the dependency array could cause unnecessary re-renders if it's not memoized in consuming components.
Consider documenting that the callback should be wrapped with
useCallback:+/** + * Hook to detect clicks outside a referenced element + * @param ref - Reference to the element + * @param callback - Function to call on outside click (should be memoized with useCallback) + * @param isDropdownOpen - Whether the dropdown is currently open + */ export function useOutsideClick<T extends HTMLElement>(src/widgets/breadcrumbs/ui/breadcrumbs.module.scss (1)
37-43: Simplify padding declarations.The mobile breadcrumb styles look good, but the padding can be streamlined.
.mobile-breadcrumbs.breadcrumbs-content { @include media-tablet-large { width: 100%; - padding: 6px; - padding-left: 0; + padding: 6px 6px 6px 0; } }src/widgets/school-menu/ui/school-item/school-item.tsx (2)
28-28: Consider using a more explicit check for non-clickable URLs.The
Boolean(url === '#')could be simplified to justurl === '#'since the comparison already returns a boolean.- const isNonClickable = Boolean(url === '#'); + const isNonClickable = url === '#';
31-31: Potential className conflict issue.Using
cx(props.className)may not properly merge class names ifprops.classNamecontains multiple classes. Consider usingcx(props.className)only if it's a single class, or handle multiple classes appropriately.- <li {...props} className={cx(props.className)}> + <li {...props} className={cx(props.className)}>Actually, this should work correctly since
cx()can handle multiple class names. The current implementation looks fine.src/widgets/mobile-view/ui/mobile-nav-item/mobile-nav-item.tsx (1)
39-45: Remove redundant role attribute.The
role="button"is redundant since the dropdown arrow is already contained within a<button>element.<span className={cx('dropdown-arrow', { rotate: isDropdownActive })} - role="button" aria-expanded={isDropdownActive} >src/widgets/mobile-view/ui/mobile-view.tsx (1)
216-228: Consider alternative approach for non-interactive itemsThe descriptive text items use
url="#"which creates non-functional links. Consider using a different component or prop to indicate these are informational items only.- <SchoolMenu.Item - className="support-title" - title="Your donations help us cover hosting, domains, licenses, and advertising for courses - and events. Every donation, big or small, helps!" - url="#" - color={color} - /> + <SchoolMenu.InfoItem + className="support-title" + title="Your donations help us cover hosting, domains, licenses, and advertising for courses + and events. Every donation, big or small, helps!" + color={color} + />src/widgets/header/ui/dropdown/dropdown-wrapper.module.scss (1)
55-68: Consider the visual impact of the decorative elementThe white
::beforeelement might not be visible against light backgrounds. Consider if this provides the intended visual effect in all scenarios.src/widgets/header/ui/nav-item/nav-item.tsx (1)
127-130: Consider moving aria-expanded to the parent buttonThe
aria-expandedattribute on a non-interactive span might be confusing. Consider moving it to the parent button element for better accessibility semantics.- {isDropdown && ( - <span className={cx('dropdown-arrow')} role="button" aria-expanded={isDropdownOpen}> - <DropdownArrow /> - </span> - )} - </button> + aria-expanded={isDropdown ? isDropdownOpen : undefined} + > + {icon && ( + <Image + src={icon} + alt={`${label} icon`} + width={18} + height={16} + aria-hidden="true" + data-testid="nav-item-icon" + /> + )} + <span className={cx('label-bold')}> + {label} + <span className={cx('label-content')} aria-hidden="true"> + {label} + </span> + </span> + {isDropdown && ( + <span className={cx('dropdown-arrow')} aria-hidden="true"> + <DropdownArrow /> + </span> + )} + </button>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
src/shared/assets/svg/heart-blue.svgis excluded by!**/*.svgsrc/shared/assets/svg/heart-yellow.svgis excluded by!**/*.svgsrc/shared/assets/svg/rss-logo-blue.svgis excluded by!**/*.svg
📒 Files selected for processing (30)
dev-data/donate-options.data.ts(1 hunks)dev-data/school-menu-links.ts(1 hunks)src/core/styles/_constants.scss(1 hunks)src/core/styles/_mixins.scss(1 hunks)src/shared/constants.ts(2 hunks)src/shared/hooks/use-outside-click/use-outside-click.ts(1 hunks)src/shared/ui/logo/logo.module.scss(1 hunks)src/shared/ui/logo/logo.tsx(2 hunks)src/shared/ui/paragraph/paragraph.module.scss(2 hunks)src/shared/ui/paragraph/paragraph.tsx(1 hunks)src/widgets/breadcrumbs/ui/breadcrumbs.module.scss(1 hunks)src/widgets/breadcrumbs/ui/breadcrumbs.tsx(2 hunks)src/widgets/footer/footer.test.tsx(2 hunks)src/widgets/header/header.module.scss(4 hunks)src/widgets/header/header.test.tsx(5 hunks)src/widgets/header/header.tsx(6 hunks)src/widgets/header/ui/dropdown/dropdown-wrapper.module.scss(2 hunks)src/widgets/header/ui/dropdown/dropdown-wrapper.tsx(1 hunks)src/widgets/header/ui/nav-item/nav-item.module.scss(3 hunks)src/widgets/header/ui/nav-item/nav-item.tsx(2 hunks)src/widgets/mobile-view/ui/mobile-nav-item/mobile-nav-item.module.scss(1 hunks)src/widgets/mobile-view/ui/mobile-nav-item/mobile-nav-item.tsx(1 hunks)src/widgets/mobile-view/ui/mobile-view.module.scss(1 hunks)src/widgets/mobile-view/ui/mobile-view.tsx(2 hunks)src/widgets/school-menu/ui/school-item/school-item.module.scss(3 hunks)src/widgets/school-menu/ui/school-item/school-item.tsx(1 hunks)src/widgets/school-menu/ui/school-menu/school-menu.module.scss(2 hunks)src/widgets/school-menu/ui/school-menu/school-menu.test.tsx(1 hunks)src/widgets/school-menu/ui/school-menu/school-menu.tsx(1 hunks)src/widgets/support/ui/support.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
dev-data/school-menu-links.ts (2)
src/shared/constants.ts (1)
ANCHORS(68-77)dev-data/index.ts (1)
communityMenuStaticLinks(49-49)
src/widgets/school-menu/ui/school-item/school-item.tsx (1)
src/shared/ui/logo/logo.tsx (1)
cx(17-17)
src/widgets/footer/footer.test.tsx (3)
src/shared/__tests__/utils/render-with-router/render-with-router.tsx (1)
renderWithRouter(38-49)src/widgets/mobile-view/ui/mobile-view.tsx (1)
MobileView(41-244)src/shared/__tests__/constants.ts (1)
mockedCourses(27-207)
src/widgets/mobile-view/ui/mobile-nav-item/mobile-nav-item.tsx (1)
src/shared/icons/dropdown-arrow.tsx (1)
DropdownArrow(5-7)
🔇 Additional comments (60)
src/shared/ui/paragraph/paragraph.module.scss (2)
18-22: Confirm spacing requirements
Unlike the other size variants,.small-font-sizedoesn’t include bottom padding. Verify against the design spec if vertical spacing is needed here.
35-37: Review responsive adjustments
At themedia-laptopbreakpoint onlyline-heightis overridden. Confirm whether properties likefont-sizeorfont-weightshould also adapt on larger screens.src/core/styles/_constants.scss (1)
56-59: Addition of intermediate gray variables
Both$color-gray-450and$color-gray-250are correctly inserted into the grayscale scale and follow the existing ordering.src/core/styles/_mixins.scss (1)
3-7: Newmedia-laptop-largemixin
The mixin appropriately targets viewports above$content-width. Ensure documentation or examples are updated to reflect this new breakpoint utility.src/shared/constants.ts (1)
45-49: ExtendedKEY_CODESfor improved keyboard interactions
AddingENTERandSPACEaligns with accessibility best practices for menu toggles.src/shared/hooks/use-outside-click/use-outside-click.ts (1)
13-17: Solid click detection logic.Good use of
mousedownevent and proper null checking before callingcontains().dev-data/donate-options.data.ts (1)
9-12: Clean separation of button and menu variants.The split into
buttonLinkLabel/menuLinkLabelandbuttonIcon/menuIconprovides good flexibility for different UI contexts.src/widgets/school-menu/ui/school-menu/school-menu.test.tsx (1)
42-44: Test expectations correctly updated.The increase from 2 to 4 elements aligns with the addition of new menu items mentioned in the PR objectives.
src/shared/ui/paragraph/paragraph.tsx (1)
18-18: LGTM! Clean addition of new font size variant.The new "small" variant follows the established pattern and integrates well with the existing CVA configuration.
src/widgets/footer/footer.test.tsx (2)
10-10: Appropriate mock addition for navigation-dependent components.The
usePathnamemock returning'/'is consistent with the testing pattern and necessary for components using Next.js navigation hooks.
41-43: Improved JSX formatting for better readability.The multi-line format makes the test props more readable while maintaining the same functionality.
dev-data/school-menu-links.ts (2)
14-23: Well-structured addition of new menu items.Both "Support Us" and "Mentors Wanted" entries follow the established pattern perfectly, using proper anchor constants and providing clear descriptions.
28-28: Improved clarity with more specific title.Changing "About" to "About Community" provides better context and reduces ambiguity when multiple "About" sections exist.
src/widgets/breadcrumbs/ui/breadcrumbs.tsx (3)
15-17: Clean type definition for enhanced styling flexibility.The optional
classNameprop follows React best practices and maintains backward compatibility.
19-19: Proper component signature update.The destructured
classNameparameter correctly implements the new prop interface.
31-31: Correct implementation of className merging.The
cx()function properly combines existing classes with the optional externalclassName, maintaining styling consistency.src/widgets/support/ui/support.tsx (2)
31-31:✅ Verification successful
Verify destructuring matches updated data structure.
The destructuring now includes
buttonLinkLabelandbuttonIconinstead of the previouslinkLabelandicon. Ensure thedonateOptionsdata structure indev-data/donate-options.data.tsexports these exact property names.
🏁 Script executed:
#!/bin/bash # Description: Verify the donateOptions data structure has the expected properties # Expected: Properties should include buttonLinkLabel and buttonIcon rg -A 10 -B 5 "buttonLinkLabel|buttonIcon" --type tsLength of output: 2744
Verification complete: destructuring matches data structure
ThedonateOptionsarray indev-data/donate-options.data.tsexportsbuttonLinkLabelandbuttonIcon, matching the destructuring insupport.tsx.
34-34:✅ Verification successful
Verify icon function call pattern.
The
buttonIcon()is called as a function. Ensure this returns a valid React component and that all icons in the data structure follow this pattern.
🏁 Script executed:
#!/bin/bash # Description: Check how buttonIcon is defined in the donate options data # Expected: Should be functions that return React components ast-grep --pattern 'buttonIcon: $$$'Length of output: 38
🏁 Script executed:
#!/bin/bash # Search for where buttonIcon is defined and how it’s assigned rg "buttonIcon" -n -C2 .Length of output: 1519
🏁 Script executed:
#!/bin/bash # Inspect imports in donate-options data sed -n '1,50p' dev-data/donate-options.data.ts # Locate component definitions for the icons rg "export .*OpenCollectiveIcon" -n . rg "export .*BoostyIcon" -n .Length of output: 1124
Icon function call pattern verified
buttonIconmaps to pure functional components (OpenCollectiveIcon,BoostyIcon) that return valid React elements and don’t use hooks, so callingbuttonIcon()is safe.(Optional) For clearer JSX semantics you could destructure:
donateOptions.map(({ buttonIcon: Icon, ... }) => ( <Icon /> … ))but no changes are required.
src/widgets/school-menu/ui/school-item/school-item.tsx (1)
39-39: Good accessibility improvement with tabIndex management.Properly setting
tabIndex={-1}for non-clickable items removes them from keyboard navigation, which improves accessibility and addresses potential keyboard navigation issues mentioned in the PR comments.src/widgets/school-menu/ui/school-item/school-item.module.scss (4)
3-4: Good spacing adjustment for better visual hierarchy.Reducing the gap from 15px to 8px and changing alignment to
flex-startcreates better visual spacing and addresses the spacing inconsistencies mentioned in PR comments.
10-13: Excellent accessibility enhancement for non-clickable items.The
.non-clickableclass properly disables pointer events and changes cursor, which works well with thetabIndex={-1}set in the component for truly non-interactive items.
57-59: Icon sizing constraint is well-implemented.Using the data-testid selector to constrain icon height to 32px ensures consistent icon sizing across the menu items.
32-32:✅ Verification successful
Verify new color variable is defined.
The
$color-gray-450variable is used but needs to be defined in the constants file.
🏁 Script executed:
#!/bin/bash # Description: Check if the new gray color variable is defined # Expected: Should find $color-gray-450 definition rg "\$color-gray-450" --type scssLength of output: 63
To catch definitions without relying on filetype detection, let’s search all SCSS files by extension:
🏁 Script executed:
#!/bin/bash # Search for any usage or definition of $color-gray-450 in .scss files rg '\$color-gray-450' -g '*.scss'Length of output: 292
Confirmed:
$color-gray-450is defined
The variable is declared insrc/core/styles/_constants.scssas:$color-gray-450: hsl(210deg 9% 62%); // #969fa8No further action needed.
src/widgets/mobile-view/ui/mobile-nav-item/mobile-nav-item.module.scss (4)
18-20: Good dropdown arrow animation implementation.The rotation animation provides clear visual feedback for collapsible menu states, which should improve the user experience mentioned in PR objectives.
1-27: Well-structured mobile navigation styling.The category link styling properly resets button defaults and provides good mobile touch targets with appropriate font sizing and spacing.
16-16:✅ Verification successful
Verify transition mixin is defined.
The
@extend %transition-allreferences a placeholder selector that should be defined in the shared styles.
🏁 Script executed:
#!/bin/bash # Description: Check if the transition-all placeholder is defined # Expected: Should find %transition-all definition rg "%transition-all" --type scssLength of output: 61
🏁 Script executed:
#!/bin/bash # Search for the %transition-all placeholder across all SCSS files rg '%transition-all' -g '*.scss' . || grep -R --include='*.scss' '%transition-all' .Length of output: 1570
%transition-all placeholder verified
The
%transition-allplaceholder is defined insrc/core/styles/_placeholders.scss, so the@extend %transition-all;inmobile-nav-item.module.scssis valid. No further changes needed.
40-40:✅ Verification successful
Verify light color variable consistency.
Using
$color-gray-100for light text. Ensure this provides sufficient contrast for accessibility.
🏁 Script executed:
#!/bin/bash # Description: Check the gray color palette definition # Expected: Should find consistent gray color variable definitions rg "\$color-gray-(100|250|450|900)" --type scssLength of output: 77
🏁 Script executed:
#!/bin/bash # Re-run search for gray color variables across all SCSS files rg -g '*.scss' '\$color-gray-(100|250|450|900)' -nLength of output: 2854
🏁 Script executed:
#!/bin/bash # Display mobile-nav-item styles to confirm background and text color context sed -n '1,200p' src/widgets/mobile-view/ui/mobile-nav-item/mobile-nav-item.module.scssLength of output: 615
Color variable usage verified
Definitions for
$color-gray-100(#f0f2f5) and$color-gray-900(#191b1d) are consistent incore/styles/_constants.scss, and the.light/.darkclasses inmobile-nav-item.module.scsscorrectly apply them. Contrast between these values meets WCAG AA/AAA on their intended backgrounds.• core/styles/_constants.scss (lines 51–61): gray palette
• src/widgets/mobile-view/ui/mobile-nav-item/mobile-nav-item.module.scss (lines 36–40):.dark/.lightclassesNo changes required.
src/widgets/mobile-view/ui/mobile-nav-item/mobile-nav-item.tsx (1)
18-47: Well-structured mobile navigation component.The component properly handles conditional icon rendering, dropdown states, and accessibility attributes. The click handler and ARIA attributes support good UX and accessibility.
src/widgets/school-menu/ui/school-menu/school-menu.tsx (2)
15-16: Good addition of layout and visibility props.These props address previous feedback about simplifying consumer component logic and moving complexity into the component itself.
28-36: Proper implementation of conditional visibility and layout.The visibility classes are applied correctly based on the
isVisibleprop, and the dynamic layout class follows React best practices.src/widgets/header/ui/dropdown/dropdown-wrapper.tsx (2)
10-10: Good addition of layout control prop.The
reverseLayoutprop provides flexibility for dropdown positioning without complex logic.
14-27: Simplified component interface improves maintainability.Removing mouse event handlers and positioning logic simplifies the component. This suggests a shift to click-based dropdown interaction, which can improve mobile UX.
src/shared/ui/logo/logo.tsx (2)
12-15: Good extension of props interface.Adding the optional
iconprop with proper TypeScript typing maintains backward compatibility while enabling dynamic logo usage.
21-31: Proper implementation of dynamic icon support.The default value ensures backward compatibility while the dynamic
srcattribute enables flexible logo rendering across different contexts.src/widgets/mobile-view/ui/mobile-view.module.scss (2)
31-34: Well-structured media query organization.Good placement of
overflow-y: hiddenand explicitflex-direction: columnin the tablet-large media query, addressing the layout structure concerns.
14-28: Excellent responsive breadcrumbs handling.The nested
.content.breadcrumbs-contentselector with tablet-specific padding removal is a clean approach for responsive design.src/widgets/header/header.test.tsx (4)
15-18: Appropriate navigation mocking for header tests.The
next/navigationmock properly stubs router functionality needed for the header component testing.
42-47: Improved test scoping with within.Using
within(desktopMenu)provides better test isolation and reduces flaky test potential compared to global queries.
90-90: Good addition of reverseLayout prop.The
reverseLayout={false}prop addition aligns with the component API changes and maintains test coverage for this feature.
68-74:❓ Verification inconclusive
Verify mobile menu arrow count aligns with implementation.
The test expects 5 SVG arrows in mobile view. Ensure this count matches the actual mobile menu structure after the refactoring.
🏁 Script executed:
#!/bin/bash # Description: Verify mobile menu dropdown arrow count in mobile-view components # Expected: Find mobile navigation items with dropdown arrows ast-grep --pattern 'aria-label="dropdown-arrow"' rg -A 3 -B 3 'dropdown-arrow' src/widgets/mobile-view/Length of output: 1986
🏁 Script executed:
#!/bin/bash # Locate the DropdownArrow icon component and inspect for aria-label DROP_PATH=$(fd dropdown-arrow.tsx) echo "Inspecting file: $DROP_PATH" # Check for any aria attributes on the icon rg -n "aria" "$DROP_PATH" # Show the SVG markup to see if an aria-label is set rg -n "<svg" -A5 "$DROP_PATH"Length of output: 374
Verify mobile menu arrow count matches rendered items
I confirmed each
<MobileNavItem>renders one icon witharia-label="dropdown-arrow". Please double-check the number of items passed into the mobile menu (insrc/widgets/header/header.tsxwhere you render the items underdata-testid="mobile-menu") and update the test’s expected length if it no longer equals 5.src/widgets/school-menu/ui/school-menu/school-menu.module.scss (4)
2-2: Good use of transition placeholder.The
@extend %transition-allprovides consistent transition behavior across the component.
29-39: Sophisticated grid layout implementation.The grid-based approach with automatic column adjustment for 5+ items is well-designed for flexible menu layouts.
52-68: Excellent visibility state implementation.The
.hiddenand.visibleclasses properly use opacity constants and create smooth transitions for show/hide behavior.
35-38:❓ Verification inconclusive
Verify grid layout behavior with variable content.
The
:has(:nth-child(5))selector switches to single-column layout. Test this behavior with different menu item counts to ensure proper responsive behavior.
🏁 Script executed:
#!/bin/bash # Description: Check school menu usage patterns to verify grid layout behavior # Expected: Find school menu components and their typical item counts rg -A 10 -B 5 'SchoolMenu.*items\|school.*menu.*data' src/ ast-grep --pattern 'SchoolMenu $$$'Length of output: 98
Verify grid layout behavior across varying menu item counts
Please ensure the single-column switch triggered by
:has(:nth-child(5))works as intended:• Test the SchoolMenu component (in app, Storybook or tests) with fewer than 5 items, exactly 5 items, and more than 5 items.
• Confirm the grid switches back to multi-column when item count drops below 5.
• Validate responsive behavior at different breakpoints.We couldn’t find automated usage patterns in the repo—manual or Storybook verification is recommended.
src/widgets/header/header.module.scss (4)
8-8: Appropriate height increase for better visual hierarchy.The navbar height increase from 64px to 70px provides better proportions and aligns with the menu refactoring goals.
14-16: Clean unified background styling.The consistent white background with subtle shadow creates a more cohesive header appearance compared to the previous color variants.
43-43: Good use of gap for menu spacing.The increased gap (33.8px) between menu items addresses previous feedback about using gap instead of margins for layout.
107-117: Well-structured support text styling.The
.support-textclass provides proper layout for donation-related content with appropriate constraints and typography.src/widgets/header/header.tsx (2)
35-38: LGTM!Clean implementation of conditional logic for mentorship page detection and icon selection.
64-130: Well-structured menu implementationGood use of NAV_MENU_LABELS constants and consistent SchoolMenu patterns across all navigation items.
src/widgets/mobile-view/ui/mobile-view.tsx (1)
49-68: Excellent state management implementationGreat use of Set for managing dropdown states and proper cleanup when menu closes. The toggle logic is clean and efficient.
src/widgets/header/ui/nav-item/nav-item.module.scss (2)
22-30: Great accessibility improvement!The addition of focus-visible styles with proper outline enhances keyboard navigation support.
53-74: Clever solution for preventing layout shiftThe nested label structure with transparent parent effectively prevents layout shift when font weight changes between regular and bold states.
src/widgets/header/ui/dropdown/dropdown-wrapper.module.scss (3)
15-17: CSS Grid implementation looks good!The three-column grid layout provides a clean structure for dropdown content organization.
29-29: Proper use of hsla color syntaxGood implementation following the requested pattern for colors with opacity.
31-53:✅ Verification successful
Verify browser support for :has() pseudo-class
The
:has()pseudo-class has limited browser support. Ensure this aligns with your browser support requirements.
🌐 Web query:
What is the current browser support for CSS :has() pseudo-class and when was it added to major browsers?💡 Result:
The CSS
:has()pseudo-class, introduced in the Selectors Level 4 specification, allows developers to select elements based on their descendants, effectively enabling parent selection in CSS. (css-tricks.com)Browser Support Timeline:
Chrome: Full support began with version 105, released on September 2, 2022. (caniuse.com)
Edge: Support was added starting with version 105, released on September 1, 2022. (caniuse.com)
Safari: Support commenced with version 15.4, released on March 14, 2022. (caniuse.com)
Firefox: Support was introduced in version 121, released on December 19, 2023. (caniuse.com)
Current Support Status (as of May 27, 2025):
As of May 2025, the
:has()pseudo-class is supported in the latest versions of major browsers:
Chrome: Versions 105 and later.
Edge: Versions 105 and later.
Safari: Versions 15.4 and later.
Firefox: Versions 121 and later.
This widespread support allows developers to utilize the
:has()pseudo-class for more dynamic and context-aware styling across modern browsers.Citations:
- 1: https://css-tricks.com/almanac/pseudo-selectors/h/has/?utm_source=openai
- 2: https://caniuse.com/css-has?utm_source=openai
- 3: https://caniuse.com/css-has?utm_source=openai
- 4: https://caniuse.com/css-has?utm_source=openai
- 5: https://caniuse.com/css-has?utm_source=openai
Browser Support for CSS :has() Verified
The CSS
:has()pseudo-class is supported in all modern browsers as of these versions:
- Chrome 105+ (Sep 2, 2022)
- Edge 105+ (Sep 1, 2022)
- Safari 15.4+ (Mar 14, 2022)
- Firefox 121+ (Dec 19, 2023)
As long as your project’s browser-support baseline targets these (or later) versions, you can safely use
:has()for dynamic grid layouts insrc/widgets/header/ui/dropdown/dropdown-wrapper.module.scss(lines 31–53). No further changes required.src/widgets/header/ui/nav-item/nav-item.tsx (4)
48-53: Good implementation of dropdown coordinationThe custom event pattern effectively ensures only one dropdown is open at a time.
70-76: Excellent keyboard accessibility implementationProper handling of Enter, Space, and Escape keys with preventDefault for form submission prevention.
80-86: Proper event listener lifecycle managementGood implementation with cleanup to prevent memory leaks.
120-125:✅ Verification successful
Verify the double label rendering pattern
The duplicate label with
aria-hiddenappears to be for styling purposes. Ensure this doesn't cause issues with screen readers or SEO.
🏁 Script executed:
#!/bin/bash # Check if this pattern is used elsewhere in the codebase ast-grep --pattern $'<span className={$_}> $_ <span className={$_} aria-hidden="true"> $_ </span> </span>'Length of output: 117
🏁 Script executed:
#!/bin/bash # Search for all occurrences of aria-hidden to identify similar double-render patterns rg -n 'aria-hidden="true"'Length of output: 1107
Double‐render label pattern is consistent and accessible
I checked similar components (mobile-nav-item, school-item, etc.) and confirmed the duplicate label with aria-hidden="true" is a deliberate styling trick. Screen readers will ignore the hidden span, so only the primary label is announced and there’s no SEO impact. No changes required.
| export const NAV_MENU_LABELS = { | ||
| RS_SCHOOL: 'RS School', | ||
| COURSES: 'Courses', | ||
| COMMUNITY: 'Community', | ||
| MENTORSHIP: 'Mentorship', | ||
| DOCS: 'Docs', | ||
| SUPPORT_US: 'Support Us', | ||
| }; |
FA27 There was a problem hiding this comment.
🛠️ Refactor suggestion
Add as const for type safety
To maintain consistency with other constant maps and preserve literal types, append as const to the NAV_MENU_LABELS definition.
🤖 Prompt for AI Agents
In src/shared/constants.ts around lines 92 to 99, the NAV_MENU_LABELS object
should be appended with 'as const' to ensure type safety and preserve literal
types. Modify the export statement by adding 'as const' at the end of the object
definition to maintain consistency with other constant maps.
|
I noticed that tapping the logo in the mobile menu doesn't trigger the onClose callback, so the collapsible menu remains open. Maybe we should add to the Logo component when it's used in the mobile menu to ensure consistent behavior. |
What type of PR is this? (select all that apply)
Description
Rework desktop and mobile menus according to the new design.
Related Tickets & Documents
Screenshots, Recordings
Before:
After:
Added/updated tests?
[optional] Are there any post deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?
Feel free to hit me with your best practices :))
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores