Align context-menu to architecture goals#198
Merged
Merged
Conversation
|
📚 Storybook preview: https://pr-198-propel-storybook.vamsi-906.workers.dev |
Add a `tone` variant (neutral/danger) to all item-row parts so destructive actions are expressed via a prop rather than ad-hoc colored children. Move the submenu trigger's popup-open highlight out of an inline cx() call into a dedicated `contextMenuSubmenuTriggerVariants` in variants.ts — component files now contain no cx() calls. Update both the ui- and components-tier stories to pass explicit tone props. Closes #125
text-danger-secondary (red-700, #e7000b) on bg-layer-1 yields 4.36:1 — just below the 4.5:1 WCAG AA threshold and fails the axe a11y gate. Promote the resting state to text-danger-primary (red-800) which passes, keeping the same token used in badge, banner and field for danger text.
The menu rows still composed their layout in the components tier with raw className spans (label, shortcut, trailing check) and baked a sized icon into the submenu trigger, so the components tier carried styling and the checkbox/ radio indicators baked in a default Check child. Add the missing item-region parts as ui parts, each rendering one element with its cva in ui/context-menu/variants.ts: - ContextMenuItemIcon — leading icon slot (node-slot, sized to the row --node-size) - ContextMenuItemLabel — the growing label - ContextMenuItemShortcut — trailing keyboard-shortcut text - ContextMenuItemIndicator — trailing single-select check slot (node-slot) - ContextMenuSubmenuTriggerIndicator — submenu caret slot (node-slot, RTL-mirrored) The checkbox/radio item indicators no longer bake a Check child or a size literal; they size whatever icon is passed via the shared node-slot class. The components tier now only composes these parts, so it holds no className, cx or cva. Stories compose the new parts and register them as subcomponents.
lifeiscontent
force-pushed
the
align/context-menu
branch
from
3016ef4 to
1a3e423
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 #125
What the spec says
From the designer's comment on #125:
Always the same — shadow/elevation, border-radius, item height and padding, separator appearance, keyboard/dismiss behavior, z-index stacking, max-height scroll, shortcut alignment, submenu arrow position, focus highlight.
Depends (adjustable) — item labels/icons, shortcut labels, groups, disabled items, submenu items, checked/radio items, and destructive items (visually distinct color).
What changed
packages/propel/src/ui/context-menu/variants.tstonevariant (neutral|danger) tocontextMenuItemVariants. This is the architectural hook for the spec's "destructive items" axis — it owns the color shift in one place rather than scatteringtext-danger-primaryacross call sites.contextMenuSubmenuTriggerVariants— a dedicated cva for the submenu trigger that bakes ingroup/itemand the popup-open highlight. This removes allcx()usage from component.tsxfiles, which now only call their cva.packages/propel/src/ui/context-menu/context-menu-{item,link-item,checkbox-item,radio-item}.tsxtoneprop and passes it to its cva call. NodefaultVariants— callers choose explicitly.packages/propel/src/ui/context-menu/context-menu-submenu-trigger.tsxcximport and inline class merging. Now callscontextMenuSubmenuTriggerVariants({ tone })directly, keeping all styling invariants.ts.packages/propel/src/components/context-menu/context-menu.stories.tsxtone="danger"instead of passingclassName="text-danger-primary"intolabelandinlineStartNodechildren.packages/propel/src/ui/context-menu/context-menu.stories.tsxContextMenuItemusages now pass an explicittoneprop.Notes
tone="neutral"items; the danger row previously relied on caller-applied color classes, which this PR replaces with a first-class prop.<Check>in CheckboxItemIndicator / RadioItemIndicator) is tracked in task Add Badge component #4 and left unchanged here — that's a broader menu-surface anatomy refactor.