[aries-core] NavMenu - Added ability to support grouping child NavItems with subheadings#5700
[aries-core] NavMenu - Added ability to support grouping child NavItems with subheadings#5700
Conversation
|
✅ Deploy Preview for hpe-theme-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for unrivaled-bublanina-3a9bae ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for hpe-design-icons-grommet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This pull request adds support for grouped navigation items in the NavigationMenu component, enabling hierarchical organization with non-interactive subheadings. The implementation introduces a new type property to distinguish between standard items and group containers, adds new NavGroup and GroupHeading components for rendering grouped structures, and includes comprehensive tests and documentation.
Changes:
- Introduced
type?: 'group' | 'item'property toNavItemTypefor distinguishing item types - Added
NavGroupandGroupHeadingcomponents with proper ARIA roles and attributes for accessibility - Updated
NavListto handle grouped navigation structures, including proper indentation and parent/child relationships - Enhanced
ItemLabelto support dynamic color changes based on interaction state - Refactored navigation stories and extracted
NavigationPanelcomponent for better code organization
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| navigation.stories.tsx | Refactored to extract NavigationPanel component and added new story for grouped navigation |
| nav-items.tsx | Added navItemsSubheadings example data demonstrating grouped navigation structure |
| NavigationPanel.tsx | New extracted component handling navigation panel logic for both mobile and desktop |
| NavList.tsx | Updated to recognize and render grouped structures with proper focus and expansion logic |
| NavItem/NavItem.tsx | Added type property to NavItemType and enhanced focus state handling |
| NavItem/ItemLabel.tsx | Added dynamic color support and conditional icon rendering |
| NavGroup/NavGroup.tsx | New component rendering group containers with accessibility attributes |
| NavGroup/GroupHeading.tsx | New component rendering non-interactive group headers with heading role |
| NavigationMenu.test.tsx | Added comprehensive tests for grouped navigation functionality |
| DOCUMENTATION.md | Updated with grouped navigation examples and ARIA guidance |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
shared/aries-core/src/js/components/core/NavigationMenu/NavItem/NavItem.tsx
Outdated
Show resolved
Hide resolved
shared/aries-core/src/js/components/core/NavigationMenu/NavGroup/NavGroup.tsx
Outdated
Show resolved
Hide resolved
shared/aries-core/src/js/components/core/NavigationMenu/NavGroup/NavGroup.tsx
Outdated
Show resolved
Hide resolved
shared/aries-core/src/js/components/core/NavigationMenu/DOCUMENTATION.md
Show resolved
Hide resolved
shared/aries-core/src/js/components/core/NavigationMenu/NavList.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
shared/aries-core/src/js/components/core/NavigationMenu/NavGroup/NavGroup.tsx
Outdated
Show resolved
Hide resolved
shared/aries-core/src/js/components/core/NavigationMenu/NavGroup/GroupHeading.tsx
Outdated
Show resolved
Hide resolved
…up/NavGroup.tsx Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
shared/aries-core/src/js/components/core/NavigationMenu/__tests__/NavigationMenu.test.tsx
Show resolved
Hide resolved
shared/aries-core/src/js/components/core/NavigationMenu/NavList.tsx
Outdated
Show resolved
Hide resolved
shared/aries-core/src/js/components/core/NavigationMenu/NavList.tsx
Outdated
Show resolved
Hide resolved
shared/aries-core/src/js/components/core/NavigationMenu/NavGroup/NavGroup.tsx
Outdated
Show resolved
Hide resolved
shared/aries-core/src/js/components/core/NavigationMenu/NavigationMenu.tsx
Outdated
Show resolved
Hide resolved
shared/aries-core/src/js/components/core/NavigationMenu/NavList.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
shared/aries-core/src/js/components/core/NavigationMenu/NavGroup/GroupHeading.tsx
Outdated
Show resolved
Hide resolved
shared/aries-core/src/js/components/core/NavigationMenu/NavList.tsx
Outdated
Show resolved
Hide resolved
shared/aries-core/src/js/components/core/NavigationMenu/NavGroup/NavGroup.tsx
Show resolved
Hide resolved
shared/aries-core/src/js/components/core/NavigationMenu/NavigationMenu.tsx
Outdated
Show resolved
Hide resolved
| ...item, | ||
| children: item.children.map(child => ({ | ||
| ...child, | ||
| level: item.level || 0, |
There was a problem hiding this comment.
here you are falling back on 0 for the level. above on line 7 isn't the type definition
export type NavItemWithLevel = NavItemType & { level?: 1 | 2 };
or am I reading this incorrectly? I know there isnt a TS error just something that caught my eye
There was a problem hiding this comment.
good eye. Fixed NavItemWithLevel type definition.
britt6612
left a comment
There was a problem hiding this comment.
can you merge master see if those tests pass
Deploy Preview
What does this PR do?
This pull request introduces support for grouped navigation items in the
NavigationMenucomponent, enhancing both its structure and accessibility. It adds new components for rendering group headers and containers, updates the navigation item data model, and includes comprehensive documentation and tests for the new grouping feature.Grouped Navigation Support:
type?: 'group' | 'item'property toNavItemTypeto distinguish between standard items and group containers, enabling hierarchical grouping in navigation menus. [1] [2]NavGroupandGroupHeadingcomponents to render group containers and accessible group headers, including proper ARIA roles and levels for improved accessibility. [1] [2] [3]NavListto recognize and render grouped navigation structures, ensuring correct indentation, focus management, and parent/child relationships. [1] [2] [3] [4] [5] [6] [7] [8]Documentation and Accessibility:
group,heading,aria-labelledby) to group containers and headers. [1] [2] [3]Testing and Storybook Integration:
NavigationPanelstory to demonstrate grouped navigation in different layouts, including mobile responsiveness. [1] [2]Other Improvements:
ItemLabelto support dynamic color changes based on item state (hover, focus, active). [1] [2] [3]These changes collectively make the navigation menu more flexible, accessible, and easier to use in complex UI scenarios.
What are the relevant issues?
Where should the reviewer start?
How should this be manually tested?
Any background context you want to provide?
Screenshots (if appropriate)
Should this PR be mentioned in Design System updates?
Is this change backwards compatible or is it a breaking change?