feat(my-pages): Navigation - new mobile menu + tweaks #17242
Conversation
WalkthroughThis pull request introduces comprehensive changes to the navigation and layout components across multiple files in the my-pages portal. The modifications focus on enhancing the responsive design of the navigation system, including updates to header positioning, adding a new mobile navigation style, and creating a more flexible Navigation component. The changes span CSS styling, component structure, and export configurations, aiming to improve the overall user interface and navigation experience. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17242 +/- ##
=======================================
Coverage 35.66% 35.66%
=======================================
Files 6941 6941
Lines 149148 149149 +1
Branches 42651 42658 +7
=======================================
+ Hits 53195 53197 +2
+ Misses 95953 95952 -1
Flags with carried forward coverage won't be shown. Click here to find out more. see 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportBranch report: ✅ 0 Failed, 84 Passed, 0 Skipped, 32.47s Total Time |
disaerna
changed the title
|
Affected services are: service-portal,system-e2e, Deployed services: service-portal. |
disaerna
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
libs/portals/my-pages/core/src/components/Navigation/Navigation.tsx (2)
229-241: Avoid rendering the title twice in theTitlecomponentIn the
Titlecomponent within theFocusableBox, you're rendering thetitletwice inside a React Fragment. BothTextcomponents display the sametitleprop with similar styles, differing only in thecolorprop. This may be unintentional and could lead to redundant rendering.Consider whether you need to display the
titletwice, or if one of theseTextcomponents should be removed or modified.Apply this diff to remove the duplicate rendering:
return ( <> <Text as="span" variant="h4" color={textColor}> {title} </Text> - <Text - as="span" - variant="h4" - color={titleLink?.active ? activeColor : color} - > - {title} - </Text> </> )
522-692: Consider breaking down theNavigationTreecomponent for better maintainabilityThe
NavigationTreecomponent spans over 170 lines and handles multiple responsibilities, including rendering navigation items, handling accordions, and managing ARIA attributes. For improved maintainability and readability, consider refactoring this component into smaller, more focused sub-components.apps/portals/my-pages/src/components/Layout/NarrowLayout.tsx (1)
100-104: Consider extracting navigation title logic.The title formatting logic is duplicated between desktop and mobile navigation. Consider extracting it to a helper function.
+const getNavigationTitle = (activeParent: PortalNavigationItem | undefined, formatMessage: FormatMessage) => ( + activeParent?.name ? formatMessage(activeParent.name) : formatMessage(m.tableOfContents) +) // Usage: -title={formatMessage(activeParent?.name ?? m.tableOfContents)} +title={getNavigationTitle(activeParent, formatMessage)}Also applies to: 130-138
libs/portals/my-pages/core/src/components/Navigation/Navigation.css.ts (1)
189-212: Consider reducing animation duration.The shake animation might be too distracting at 0.5s duration. Consider reducing it to 0.3s for a subtler effect.
- animation: `${shakeKeyframe} 0.5s`, + animation: `${shakeKeyframe} 0.3s`,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/portals/my-pages/src/components/Header/Header.css.ts(2 hunks)apps/portals/my-pages/src/components/Layout/Layout.css.ts(1 hunks)apps/portals/my-pages/src/components/Layout/NarrowLayout.tsx(4 hunks)libs/portals/my-pages/core/src/components/Navigation/Navigation.css.ts(1 hunks)libs/portals/my-pages/core/src/components/Navigation/Navigation.tsx(1 hunks)libs/portals/my-pages/core/src/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/portals/my-pages/src/components/Header/Header.css.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/portals/my-pages/src/components/Layout/Layout.css.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/portals/my-pages/core/src/index.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
apps/portals/my-pages/src/components/Layout/NarrowLayout.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/portals/my-pages/core/src/components/Navigation/Navigation.css.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/my-pages/core/src/components/Navigation/Navigation.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (6)
apps/portals/my-pages/src/components/Header/Header.css.ts (1)
Line range hint
10-21: Verify the impact of changing the header position fromfixedtorelativeon mobileThe
headerstyle now setsposition: 'relative'by default andposition: 'fixed'on larger screens. Changing the header position torelativeon mobile devices may affect the layout and behavior of other components that assume the header is fixed.Please verify that this change does not introduce layout issues or negatively affect the user experience on mobile devices.
apps/portals/my-pages/src/components/Layout/Layout.css.ts (1)
24-29: Ensure that thezIndexvalue does not conflict with other componentsThe
mobileNavstyle introduces azIndexof99. Please verify that thiszIndexvalue does not conflict with other elements on the page, such as modals or overlays, and that it maintains the desired stacking order across the application.libs/portals/my-pages/core/src/index.ts (1)
49-49: LGTM! Export follows established patterns.The new Navigation component export follows the existing pattern and maintains good organization.
apps/portals/my-pages/src/components/Layout/NarrowLayout.tsx (2)
9-9: LGTM! Import organization improved.The Navigation import is now properly grouped with other core imports.
Line range hint
120-139: Verify mobile navigation accessibility.The mobile navigation implementation includes the isMenuDialog prop and styling, but we should ensure it meets accessibility standards.
Run the following to check for accessibility attributes:
✅ Verification successful
Mobile navigation meets accessibility standards
The mobile navigation implementation includes comprehensive accessibility features:
- Proper ARIA roles and attributes (menu, menuitem, aria-expanded, aria-controls)
- Keyboard navigation support through FocusableBox
- Screen reader support with VisuallyHidden text and semantic HTML
- Focus management with visual indicators
- Accessible dialog implementation using ModalBase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for dialog-related ARIA attributes in Navigation component ast-grep --pattern 'role="dialog"' ast-grep --pattern 'aria-modal="true"'Length of output: 77
Script:
#!/bin/bash # Find Navigation component implementation fd -e tsx -e ts "Navigation" --type f # Search for accessibility-related code rg -i "aria|role.*dialog" -A 3 -B 3 # Look for menu-related accessibility attributes ast-grep --pattern 'role="menu"' ast-grep --pattern 'aria-label'Length of output: 65920
Script:
#!/bin/bash # Check Navigation component implementation cat libs/portals/my-pages/core/src/components/Navigation/Navigation.tsx # Look for keyboard handlers and focus management rg -i "keydown|focus" libs/portals/my-pages/core/src/components/Navigation/Navigation.tsxLength of output: 20569
libs/portals/my-pages/core/src/components/Navigation/Navigation.css.ts (1)
182-187: Review z-index value for potential conflicts.The z-index of 1500 for mobile navigation is quite high. Verify this doesn't conflict with other overlays or modals.
✅ Verification successful
Z-index value of 1500 is appropriate
The current z-index fits well within the codebase's established z-index hierarchy, sitting between regular navigation components (1000) and modal overlays (10000+). No conflicts expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other high z-index values that might conflict rg "z-index.*(?:1[0-9]{3}|[2-9][0-9]{3})" -g "*.{ts,tsx,css}"Length of output: 63
Script:
#!/bin/bash # Search for z-index values with various patterns echo "=== Searching for z-index in style objects ===" rg "zIndex: *\d{4,}" -g "*.{ts,tsx}" rg "zIndex: *[1-9]\d{3}" -g "*.{ts,tsx}" echo -e "\n=== Searching for z-index in CSS styles ===" rg "z-index: *\d{4,}" -g "*.{ts,tsx,css}" rg "z-index: *[1-9]\d{3}" -g "*.{ts,tsx,css}"Length of output: 2947
libs/portals/my-pages/core/src/components/Navigation/Navigation.tsx
Outdated
Show resolved
Hide resolved
libs/portals/my-pages/core/src/components/Navigation/Navigation.css.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/portals/my-pages/src/components/Layout/Layout.css.ts (1)
24-28: Consolidate z-index values across navigation styles.The z-index value (99) differs from the one in Navigation.css.ts (1500). Consider:
- Using a shared constants file for z-index values
- Documenting the z-index hierarchy
- zIndex: 99, + zIndex: theme.zIndex.navigation, // Add this to theme
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/portals/my-pages/src/components/Header/Header.css.ts(2 hunks)apps/portals/my-pages/src/components/Layout/Layout.css.ts(1 hunks)apps/portals/my-pages/src/components/Layout/NarrowLayout.tsx(4 hunks)libs/portals/my-pages/core/src/components/Navigation/Navigation.css.ts(1 hunks)libs/portals/my-pages/core/src/components/Navigation/Navigation.tsx(1 hunks)libs/portals/my-pages/core/src/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
libs/portals/my-pages/core/src/index.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
apps/portals/my-pages/src/components/Header/Header.css.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/portals/my-pages/src/components/Layout/NarrowLayout.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/portals/my-pages/src/components/Layout/Layout.css.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/portals/my-pages/core/src/components/Navigation/Navigation.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/my-pages/core/src/components/Navigation/Navigation.css.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (6)
apps/portals/my-pages/src/components/Header/Header.css.ts (1)
10-10: Responsive header positioning is appropriateSetting the
positionto'relative'by default and'fixed'for larger screens ensures that the header behaves responsively. This change allows the header to scroll with the content on smaller screens, enhancing the mobile user experience, while keeping it fixed on larger screens.Also applies to: 21-21
libs/portals/my-pages/core/src/index.ts (1)
49-49: Exporting theNavigationcomponent enhances reusabilityThe addition of
export * from './components/Navigation/Navigation'makes theNavigationcomponent available for import across different NextJS apps. This aligns with the reusability guidelines and promotes modular design within the codebase.apps/portals/my-pages/src/components/Layout/NarrowLayout.tsx (2)
2-2: LGTM! Import changes align with the new navigation component.The Navigation component is now correctly imported from my-pages/core, which aligns with the PR objective to create a custom navigation for My pages.
Also applies to: 9-10
Line range hint
120-139: Verify mobile navigation accessibility.The mobile navigation implementation looks good and aligns with the PR requirements. However, ensure that:
- The menu dialog is keyboard accessible
- Proper ARIA attributes are set for the expandable menu
- Focus management is handled correctly when the menu opens/closes
Run this script to check for accessibility attributes:
✅ Verification successful
Mobile navigation accessibility implementation verified.
The mobile navigation implementation has proper accessibility support with:
- Keyboard accessibility through MenuButton/Menu components
- ARIA attributes: role="menu", role="menuitem", aria-label
- Focus management via ModalBase and Menu components
- Screen reader support with proper labeling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for accessibility-related attributes in Navigation component ast-grep --pattern 'aria-$_="$_"'Length of output: 35
Script:
#!/bin/bash # Search for accessibility-related patterns echo "=== Checking role attributes ===" ast-grep --pattern 'role="$_"' echo -e "\n=== Checking aria attributes with ripgrep ===" rg "aria-" -A 2 -B 2 echo -e "\n=== Checking keyboard event handlers ===" rg "onKey(Down|Up|Press)" -A 2 -B 2 echo -e "\n=== Checking focus management ===" rg "focus" -A 2 -B 2 echo -e "\n=== Looking for Navigation component implementation ===" ast-grep --pattern 'const Navigation = $_'Length of output: 73772
Script:
#!/bin/bash # Search for dialog/menu related patterns echo "=== Checking dialog/menu roles ===" rg "role=(\"|')?dialog|menu" -A 2 -B 2 echo -e "\n=== Checking dialog/menu related components ===" rg -l "(Dialog|Menu|Navigation)" --type ts --type tsx echo -e "\n=== Checking focus trap utilities ===" rg -l "focusTrap|FocusTrap|useFocusTrap" --type ts --type tsx echo -e "\n=== Checking menu-related ARIA attributes ===" rg "aria-(expanded|haspopup|controls|labelledby|describedby)" -A 2 -B 2Length of output: 70954
libs/portals/my-pages/core/src/components/Navigation/Navigation.css.ts (2)
189-212: LGTM! Shake animation implementation matches design requirements.The shake animation is well-implemented with proper keyframes and timing, matching the PR requirement for menu icon shake after one minute of inactivity.
182-187: Review z-index value for potential conflicts.The z-index value of 1500 seems arbitrarily high. Consider:
- Documenting the z-index strategy
- Ensuring this value doesn't conflict with other overlays/modals
Run this script to check for other z-index values:
✅ Verification successful
Z-index value of 1500 is appropriate for mobile navigation
The value is well-positioned in the existing z-index hierarchy:
- Below critical overlays (filters: 10000, shadows: 9999)
- Above standard UI elements (modals: 2, menus: <1000)
- Ensures mobile navigation remains accessible while not interfering with higher-priority overlays
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all z-index declarations in CSS files rg -i "z-index:\s*\d+" --type cssLength of output: 865
Script:
#!/bin/bash # Search for z-index in CSS-in-JS files within the portals directory rg -i "z-?index.*[:=]" libs/portals -A 1 # Also search for potential theme.zIndex references rg -i "theme\.z" libs/portalsLength of output: 12311
libs/portals/my-pages/core/src/components/Navigation/Navigation.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
libs/portals/my-pages/core/src/components/Navigation/Navigation.tsx (2)
69-132: Consider strengthening type definitions for better type safety.While the interfaces are well-structured, consider these improvements:
NavigationItem['slug']could be more specific thanstring[]NavigationItem['typename']could be an enum or union typeLeveltype could be used inNavigationTreeProps['level']instead of being optional
647-647: Remove commented-out code.Remove the commented-out background prop as it adds noise to the codebase.
- //background={colorSchemeColors[colorScheme]['dividerColor']}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/portals/my-pages/core/src/components/Navigation/Navigation.css.ts(1 hunks)libs/portals/my-pages/core/src/components/Navigation/Navigation.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/portals/my-pages/core/src/components/Navigation/Navigation.css.ts
🧰 Additional context used
📓 Path-based instructions (1)
libs/portals/my-pages/core/src/components/Navigation/Navigation.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: tests (web)
- GitHub Check: tests (service-portal)
- GitHub Check: tests (portals-my-pages-signature-collection,portals-my-pages-social-insurance-maintenance)
- GitHub Check: tests (portals-my-pages-petitions,portals-my-pages-sessions)
- GitHub Check: tests (portals-my-pages-information,portals-my-pages-law-and-order)
- GitHub Check: tests (portals-my-pages-finance,portals-my-pages-health)
- GitHub Check: tests (portals-my-pages-education-license,portals-my-pages-education-student-assessment)
- GitHub Check: tests (portals-my-pages-education-career,portals-my-pages-education-degree)
- GitHub Check: docker-build ({"projects":"system-e2e","docker_type":"docker-playwright"})
- GitHub Check: helm-docker-build
- GitHub Check: docker-build ({"projects":"service-portal","docker_type":"docker-static"})
- GitHub Check: tests (my-pages-air-discount)
- GitHub Check: docker-build ({"projects":"web","docker_type":"docker-next"})
- GitHub Check: build (web)
🔇 Additional comments (3)
libs/portals/my-pages/core/src/components/Navigation/Navigation.tsx (3)
1-68: LGTM! Well-structured type definitions and context setup.The type definitions and color scheme mapping follow TypeScript best practices, with proper typing for the NavigationContext and comprehensive color scheme definitions.
230-242: Remove duplicate title rendering.The title is rendered twice within the fragment, which is unnecessary and could lead to confusion.
362-423: LGTM! Well-implemented mobile navigation dialog.The component follows accessibility best practices with proper ARIA attributes and modal management.
My pages - Navigation
What
Add copy of Navigation component to My pages - Core. Why? Because we need new stuff that is impossible to to across the whole project.... at least for now 🤡
Mobile menu is now sticky on top and opens full screen when opened
Navigation has new chevron icons
Parent item can be opened when clicking on the parent without rerouting
Mobile menu icon shakes when the user has been idle for 15 sec (request from designer)
.
Screenshots / Gifs
Mobile menu is now bigger
Before:
After:
Mobile menu is now sticky
Before:
After:
Mobile menu is fullsize when opened and parent item can be opened without rerouting to first child
Before:
After:
Menu in desktop and tablet has bigger font
Before:
After:
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates