fix(ui): fix invalid HTML structure in SideNavigation component tree#1792
Merged
Conversation
Signed-off-by: Franz Heidl <f.heidl@sap.com>
|
Contributor
|
franzheidl
added
the
ui-components
Signed-off-by: Franz Heidl <f.heidl@sap.com>
- remove Level context from SideNavigationGroup - add top spacing to SideNavigationGroup (except for first-child) - decrease font-size for -Group label Signed-off-by: Franz Heidl <f.heidl@sap.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes invalid HTML list semantics in the SideNavigation component tree by ensuring that list containers (<ul>) contain proper list items (<li>) and that expandable nested items render inside nested <ul> elements, improving spec compliance and assistive-technology semantics.
Changes:
- Updated
SideNavigationItemto render a<li>root and wrap expanded nested items in a<ul>. - Updated
SideNavigationGroupto render a<li>root and wrap expanded children in a<ul>. - Adjusted Storybook examples and added minor SideNavigation CSS updates related to groups.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui-components/src/components/SideNavigationItem/SideNavigationItem.component.tsx | Converts item wrapper to <li> and introduces nested <ul> for expanded children. |
| packages/ui-components/src/components/SideNavigationGroup/SideNavigationGroup.component.tsx | Converts group wrapper to <li> and introduces nested <ul> for expanded children. |
| packages/ui-components/src/components/SideNavigationGroup/SideNavigationGroup.stories.tsx | Updates group stories to reflect new rendering / nesting behavior. |
| packages/ui-components/src/components/SideNavigation/SideNavigation.stories.tsx | Updates SideNavigation story examples to use the new group/item nesting patterns. |
| packages/ui-components/src/components/SideNavigation/sidenavigation.css | Adds/adjusts group-related styling and comments. |
- Guard nested <ul> rendering on hasChildren so empty lists are not emitted - Reset default list styling for all <ul>s inside .juno-sidenavigation - Update stories description and fix typo in sidenavigation.css comment - Rewrite stale level-* tests to match groups-are-top-level behavior - Add regression tests asserting valid <ul>/<li> list semantics Signed-off-by: Esther Schmitz <esther.schmitz@sap.com>
- add class to component - fix test accrodingly Signed-off-by: Franz Heidl <f.heidl@sap.com>
edda
approved these changes
Jun 24, 2026
ArtieReus
approved these changes
Jun 24, 2026
1 task
edda
added a commit
that referenced
this pull request
Jun 24, 2026
Covers PR #1792, which was merged without a changeset. Signed-off-by: Esther Schmitz <esther.schmitz@sap.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The SideNavigation component tree was producing invalid HTML as per issue #1791:
SideNavigationrenders a<ul>but its children (SideNavigationItem,SideNavigationGroup) were rendering<div>wrappers and React fragments instead of<li>elements. Direct children of a<ul>must be<li>elements; anything else violates the HTML spec and breaks assistive technology semantics.A second related issue: when
SideNavigationItemhad nested children (expand/collapse), they were rendered as flat siblings after the item rather than inside a nested<ul>.Changes
SideNavigationItem<div>replaced with<li>(withflex-colto stack the header row and the nested sub-list)<ul>when expandedchildrenprop JSDoc updated to clarify primary use (nested items) vs. string fallback (label)SideNavigationGroup<li><ul>when expandedBehavior change:
SideNavigationGroupis top-level onlySideNavigationGrouppreviously consumed and incrementedLevelContext, which caused its label and descendants to be indented based on nesting depth. As part of this PR,SideNavigationGroupno longer participates inLevelContext:level-*class — groups always render flush with the navigation edge.SideNavigationItemchildren render atlevel-1.This reflects the intended use of
SideNavigationGroupas a first-level grouping construct only. Nesting groups inside groups (or inside items) is not a supported pattern; use nestedSideNavigationItems instead for hierarchical navigation.Notes
<ul>(rather than require callers to pass one) was deliberate:childrenis already typed toReactElement<SideNavigationItemProps>, so the component can safely wrap unconditionally with no risk of producing a<ul><ul>double-wrap. TypeScript enforces call-site correctness; the auto-wrap keeps the API clean and leak-free.SideNavigationItemnesting insideSideNavigationItemis unaffectedRelated Issues
Closes #1791 , #1787