Fix NavList indentation - #1411
Conversation
🦋 Changeset detectedLatest commit: 0a0c699 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🟢 No design token changes found |
🟢 No unit test coverage changes foundAll components and hooks with tests maintain the same coverage as the main branch. |
🟢 Bundle size report
|
🟢 No visual differences foundOur visual comparison tests did not find any differences in the UI. |
Copilot-Session: 179d54c8-0b59-4769-a234-92919fb93779
There was a problem hiding this comment.
Pull request overview
This PR aims to correct NavList leaf-item indentation when using grouped nested disclosure levels, and adds Storybook + visual regression coverage to prevent regressions.
Changes:
- Adds grouped-nesting-specific indentation rules for level 3–5 items in
NavList.module.css. - Introduces a new
GroupedNestedLevelsfeature story demonstrating grouped nesting through level 5. - Adds a Playwright visual spec entry and a changeset for the patch release.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/NavList/NavList.module.css | Adds grouped-list-specific padding rules for nested levels to correct indentation. |
| packages/react/src/NavList/NavList.features.stories.tsx | Adds a new Storybook feature story to display grouped nesting through level 5. |
| packages/react/src/NavList/NavList.visual.spec.ts | Adds a new visual regression test for the new grouped nested levels story. |
| .changeset/neat-lists-align.md | Declares a patch release note for the indentation fix. |
Review details
Comments suppressed due to low confidence (3)
packages/react/src/NavList/NavList.module.css:380
- Similar to the level-3 grouped rule, the level-4 grouped indentation is hard-coded (32px + 2px). Basing this on
--brand-NavList-item-paddingInlineStart-level-2keeps the 16px-per-level relationship explicit and resilient to future changes.
.NavList__groupList .NavList__item--level-4 > .NavList__itemContent {
padding-inline-start: calc(var(--base-size-32) + var(--base-size-2));
}
packages/react/src/NavList/NavList.module.css:384
- Grouped leaf level-4 indentation is currently fixed at
var(--base-size-32), which ignores the--brand-NavList-alignedLabel-paddingInlineStartbase used by grouped level-2 leaves. If that base stays atvar(--base-size-2), this makes the leaf level spacing deviate from the intended 16px steps. Consider deriving this from the aligned-label custom property.
.NavList__groupList .NavList__item--leaf.NavList__item--level-4 > .NavList__itemContent {
padding-inline-start: var(--base-size-32);
}
packages/react/src/NavList/NavList.module.css:388
- Grouped leaf level-5 indentation is fixed at
var(--base-size-48), which similarly bypasses the--brand-NavList-alignedLabel-paddingInlineStartbase that affects grouped leaf level-2 indentation. Deriving from that base keeps the per-level spacing consistent when the base changes.
.NavList__groupList .NavList__item--leaf.NavList__item--level-5 > .NavList__itemContent {
padding-inline-start: var(--base-size-48);
}
- Files reviewed: 4/6 changed files
- Comments generated: 3
- Review effort level: Low
Copilot-Session: 179d54c8-0b59-4769-a234-92919fb93779
|
|
||
| .NavList__groupList .NavList__item--leaf.NavList__item--level-5 > .NavList__itemContent { | ||
| padding-inline-start: calc( | ||
| var(--brand-NavList-alignedLabel-paddingInlineStart, var(--base-size-24)) + var(--base-size-32) + var(--base-size-8) + |
There was a problem hiding this comment.
@danielguillan could you help me understand what these fallbacks are for when we already have a token for it? In this example, it seems to be computing to 70px but that feels oddly specific (and not in our base size scales). Assuming you do need a precise 70px value, is there a reason we can't make this shorter like --base-size-64+ --base-size-6 (or similar)? I'm probably missing something really obvious here 😄.
Another reason for asking this is that I'm trying to understand how each item lines up against it's parent or descendent. Right now they feel a bit off still.
There was a problem hiding this comment.
@rezrah I've simplified the offsets and removed the redundant fallbacks. Should work better now and they shouldn't feel off anymore.
rezrah
left a comment
There was a problem hiding this comment.
Yes, looks good to me now @danielguillan. Thanks for fixing that, it's lining up much better 📐
Copilot-Session: 5600da7c-00a7-4277-af48-1773f75eaad9
Summary
Fixes
NavListindentation across grouped nested disclosure levels.List of notable changes:
What should reviewers focus on?
NavListfeature stories remain unchanged.Steps to test:
Nested DisclosureandGrouped Nested LevelsunderComponents/NavList/Features.Screenshots: