Align menu to architecture goals#189
Open
lifeiscontent wants to merge 3 commits into
Open
Conversation
|
📚 Storybook preview: https://pr-189-propel-storybook.vamsi-906.workers.dev |
- Move all className composition from component .tsx files into cva variants.ts: MenuCheckboxItem and MenuSubTrigger previously used inline cx() calls; MenuSeparator and MenuViewport had hardcoded className strings. All styling now lives in named cva exports (menuCheckboxItemVariants, menuSubTriggerVariants, menuSeparatorVariants, menuViewportVariants). - Drop the standalone menuItemVariants export (was only used by MenuLinkItem and MenuRadioItem). Both parts now call menuRowVariants with their fixed axes baked in (link: emphasis="link"; radio: emphasis="default"), giving them the group/item context class needed for child disabled-state selectors in compositions. - Make MenuCheckboxItemIndicator and MenuRadioItemIndicator single-element parts: removed the baked-in default <Check /> child. Parts render exactly one element; callers pass their icon as children (the ui-tier story already did this). - Move the multi-element MenuLabel (GroupLabel + two spans for truncate + inlineEnd) from the ui tier to the components tier where composition belongs. The ui-tier MenuLabel is now a single-element GroupLabel wrapper. A new components/menu/menu-label.tsx carries the inlineEndNode composition, and the components index now exports MenuLabel locally instead of re-exporting from ui. The menuLabelVariants cva is added to variants.ts for the components-tier version.
Each menu UI part now renders exactly one element. The item rows' internal layout — the leading icon, the title column, the title/secondary line, the description, trailing meta/slot, the single-select check, the submenu chevron, the checkbox control, and the label title/meta — are their own named parts with cva pairings in ui/menu/variants.ts, mirroring the accordion anatomy. The components tier no longer carries any styling: MenuItem, MenuCheckboxItem, MenuLinkItem, MenuSubTrigger, MenuLabel, MenuSearch and MenuFooter now compose those UI parts with no className/cva/cx. MenuContent composes the MenuPortal/ MenuPositioner/MenuPopup parts instead of inline classes. MenuPopup gains a required `surface` axis (raised standalone chrome vs inset padding inside an OverlayPanel), replacing the duplicated popup classes; menubar and the UI story pass it explicitly. The UI story composes the new atomic parts and documents them as subcomponents.
The selected, submenu, and search-icon menu ui parts baked a default glyph via children ?? <Icon/>. Make each render only its child so a ui part is a pure slot; move the default glyph into the components-tier compositions (MenuItem, MenuSubTrigger, MenuSearch) and the ui story.
lifeiscontent
force-pushed
the
align/menu
branch
from
830ce4a to
833ee02
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 #134
What the designer's spec says
From the issue comment, the menu has two categories of concerns:
Always the same (baked in, not configurable): shadow/elevation, border-radius, item height and horizontal padding, separator style, keyboard navigation, focus/hover highlight, dismiss behavior, max-height with scroll, icon column alignment, shortcut label alignment, indicator position, submenu chevron position.
Depends / adjustable (exposed as props or composition slots): item label, icon, keyboard shortcut, groups and separators, disabled state, submenus, checked/radio state, destructive color, item descriptions.
The component already had most of the "always the same" items baked into Tailwind classes. This PR focuses on making sure those classes live in the right place (cva variants.ts, not inline in component files) and that parts follow the single-element rule.
Architecture changes
Styling into cva (
variants.ts)MenuCheckboxItemandMenuSubTriggerwere using inlinecx(...)calls inside their.tsxfiles. Moved tomenuCheckboxItemVariantsandmenuSubTriggerVariantsinvariants.ts.MenuSeparatorhad a hardcodedclassNamestring; now callsmenuSeparatorVariants().MenuViewporthad a hardcoded"relative"; now callsmenuViewportVariants().menuItemVariantsexport (a flat row base without group context) is removed; it was only used byMenuLinkItemandMenuRadioItem.Row parts use
menuRowVariantsconsistentlyMenuLinkItemandMenuRadioItemwere using the old flatmenuItemVariants, which lacked thegroup/itemcontext class. Without it, child disabled-state selectors (group-data-disabled/item:text-disabled) in composed rows don't fire. Both now callmenuRowVariantswith their axes baked in (link:emphasis="link"for cursor-pointer; radio:emphasis="default").Single-element rule: indicators
MenuCheckboxItemIndicatorandMenuRadioItemIndicatorbaked in a default<Check />child, rendering two elements when no children were passed. Removed the default. Callers pass their own icon as children — the ui-tier story already did this explicitly.Single-element rule:
MenuLabelmoved to components tierThe old ui-tier
MenuLabelrendered aGroupLabelwrapping two spans (one for the truncated text, one for the optional inline-end badge). Composition belongs at the components tier. The ui-tierMenuLabelis now a single-elementGroupLabelwrapper. A newcomponents/menu/menu-label.tsxcarries theinlineEndNodelayout.menuLabelVariantsis added tovariants.tsfor this. The componentsindex.tsxnow exportsMenuLabellocally instead of re-exporting from the ui tier.Notes
vp check --no-fmtandvp check --fixwith no errors.MenuLabelfrom the components tier, which still hasinlineEndNodesupport.