fix(sidebar): align icon-less labels and stop a root link stealing the highlight - #2130
Merged
Merged
Conversation
Stripping the base URL's path from the destination leaves nothing behind for a link to the site's own home, and `path.Clean` renders that empty string as ".". Only the empty form was caught by the emptiness guard, so "." survived as a relative page reference meaning "the page being rendered" and the lookup that follows resolved it to the caller. The sidebar is where this surfaces. Its markup is cached per section against the section root, so a menu entry linking to "/" rendered as a link to the section root, then matched the current page in the client-side active pass and took the highlight from the entry that owns it. Add assertions for assets/link.html to the template test site, covering the root case plus the absolute, section and anchored links either side of it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A menu is free to mix entries that lead with an icon and entries that do not, and the ones without then started their label at the padding edge while their neighbours started an icon-width further right, breaking the column of labels. Resolve per sibling list whether anything in it carries an icon, and mark the rows that do not with `sidebar-item-icon-indent`, which reserves the same column. Scoped to the list rather than the whole sidebar: a nested list of icon-less entries is aligned already, and reserving a column there would indent it away from the label it hangs under - the shape of every menu that puts icons on its top level only. The class comes from the template rather than a CSS `:has()` derivation, for the reason already documented for `sidebar-nav-icon-indent`: the collapse trail is expanded client side, and a selector re-resolved as a subtree becomes visible can paint the wrong value first. Add assertions covering a mixed list, a list without icons, a list where every row has one, and nested lists in both directions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for gethinode-demo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Collaborator
Author
|
🎉 This PR is included in version 3.21.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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
1 participant
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.
Two sidebar defects, reported against a downstream app whose menu mixes entries with and without icons and carries a
link: "/"home entry.A link to the site root resolves to the calling page
assets/link.htmlnormalizes its destination by stripping the base URL's path from it. For a link to the site's own home that leaves the empty string, andpath.Cleanrenders the empty string as".". Only the empty form was caught by the emptiness guard, so"."survived into the page lookup — where it is a valid relative reference meaning "the page being rendered".The sidebar is what makes it visible. Its markup is cached per section against the section root, so a menu entry linking to
/rendered ashref="/project/"on the project section,href="/monitor/"on monitor, and so on. That entry then matched the current page exactly insidebar-active.js— where the first exact match wins — and took the highlight from the entry that owns the page.The guard now treats
"."as the site root alongside the empty string.Entries without an icon do not line up with entries that have one
A row without an icon started its label at the padding edge while its neighbours started an icon-width further right, so the labels no longer formed a column and the odd row read as misplaced rather than as merely unadorned.
assets/sidebar.htmlnow resolves, per sibling list, whether anything in that list carries an icon, and marks the rows that do not withsidebar-item-icon-indent; the stylesheet turns that into a reservation of the same column. The scope is the sibling list rather than the whole sidebar on purpose — a nested list of icon-less entries is aligned already, and reserving a column there would indent it away from the label it hangs under, which is the shape of every menu that puts icons on its top level only. The class is set by the template rather than derived in CSS with:has(), for the reason already documented forsidebar-nav-icon-indent.Sites that render no icons, or icons on every row of a list, produce byte-identical markup.
Tests
Adds assertions to the template test site for both: four
assets/link.htmlcases (root, absolute, section, anchored) and five sidebar cases (mixed list, no icons, all icons, and nested lists in both directions). The harness gains mounts forassets/link.html,assets/sidebar.html, mod-fontawesome'sicon.htmland the icon set, so the rows and their icons are rendered for real rather than stubbed.Reverting both fixes turns these into 5 failures; with them,
npm testis clean and the theme's own site builds unchanged.Verification
Checked in a browser against a consuming app built with this branch:
/(the home entry, exact), on a section root, and on an L2 page (L1 ancestor + L2 exact)🤖 Generated with Claude Code