Align navigation-menu to architecture goals#179
Open
lifeiscontent wants to merge 3 commits into
Open
Conversation
|
📚 Storybook preview: https://pr-179-propel-storybook.vamsi-906.workers.dev |
Replace the baked `size-4` container on NavigationMenuIcon with the shared node-slot pattern (`[&>svg]:size-(--node-size)`, defaulting `--node-size` to 1rem). Callers no longer need to pass `className="size-3.5"` on the child SVG — the container inherits the token and applies it to its direct SVG child automatically. Update both the UI-tier and components-tier stories to drop the now- redundant size class on the ChevronDown icon.
…tyling Adds the missing anatomy extensions so every ui part renders a single element and the components tier only composes parts: - NavigationMenuTriggerLabel: the trigger's text label beside the Icon - NavigationMenuContentList: the vertical <ul> stack inside a Content panel - NavigationMenuLinkTitle / NavigationMenuLinkDescription: the title and optional description lines of a rich content link Link arrangement is now a required variant (an inline item pill vs a stacked content card) instead of one overloaded shape, so the chrome lives in one cva. Both stories compose the new parts and register them as subcomponents; the raw <ul>/<span> classNames that had leaked into the components-tier story are gone.
The Default story's first product-links list opened as <ul> but closed as </NavigationMenuContentList>; replace the wrong closing tag with </ul> in both ui/ and components/ story files.
lifeiscontent
force-pushed
the
align/navigation-menu
branch
from
f452643 to
f95a41e
Compare
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.
Closes #136
What the designer specified
From the design system review, these properties are always the same (baked in):
inline-flex h-8 items-center gap-1)data-popup-open:bg-layer-transparent-selected)px-3)gap-1)focus-visible:ring-2 focus-visible:ring-accent-strong)transition-transform)These are depends / adjustable (supplied by the caller):
data-popup-openstateNavigationMenuContent)Architecture changes
NavigationMenuIconnode-slot pattern — the previous implementation usedflex size-4 items-center justify-centeron the container, which forced callers to specifyclassName="size-3.5"on the child SVG (e.g.,<ChevronDown aria-hidden className="size-3.5" />). That violates the architecture rule: sized children should be driven by a CSS custom property on the container, not baked onto the child by the caller.The new implementation aligns with the
nodeSlotClasspattern used byAccordionTriggerIconand other parts:inline-flex shrink-0 items-center justify-center [--node-size:1rem] [&>svg]:size-(--node-size)<ChevronDown aria-hidden />— no size class neededThe container's
--node-sizedefault (1rem) matches the trigger's line-height context, so the icon reads at the right optical size without any caller-side class.Both the UI-tier and components-tier stories are updated to drop the now-redundant
className="size-3.5"on the ChevronDown icon.Notes
Needs design approval from @bhaveshraja before merging.