Align breadcrumb to architecture goals#203
Open
lifeiscontent wants to merge 3 commits into
Open
Conversation
|
📚 Storybook preview: https://pr-203-propel-storybook.vamsi-906.workers.dev |
Move all className composition into cva in variants.ts (ui and components tiers), remove every cx() call from component files, add new variants for BreadcrumbItem / BreadcrumbSeparator / the ordered list, merge the two separate trigger cva calls into one, move BreadcrumbPage's text-primary into the interactive:false variant, move BreadcrumbDropdownItem's className into a cva, and replace the icon <span> wrapper in BreadcrumbMenuTrigger with a named slot variant. Separator now sizes its default chevron via --node-size / [&>svg] instead of a hardcoded className on the child, and RTL mirroring is baked into the separator variant. Closes #121.
Split the multi-element ui parts into single-element parts and lift all styling out of the components tier: - Breadcrumb now renders only <nav>; the <ol> is a new BreadcrumbList part. - New BreadcrumbTriggerIcon (leading icon slot) and BreadcrumbTriggerIndicator (trailing chevron) ui parts, each a single element with cva in variants.ts, replacing the inline icon/chevron baked into BreadcrumbMenuTrigger. - BreadcrumbMenuTrigger now composes the ui trigger + icon + indicator parts with no styling. - BreadcrumbDropdown reuses MenuContent and BreadcrumbDropdownItem reuses MenuLinkItem, so the components tier no longer owns popup/item chrome. - Deleted components/breadcrumb/variants.ts; the components tier holds no cva, cx, or className literals. - Stories compose the new parts and register them in subcomponents.
All seven variant functions in packages/propel/src/ui/breadcrumb/variants.ts were prefixed `crumb*` instead of `breadcrumb*`. Renamed each to match its consuming part exactly per rule 3a. The shared crumbVariants (used by both BreadcrumbLink and BreadcrumbPage with different `interactive` values) is split into breadcrumbLinkVariants and breadcrumbPageVariants; the interactive variant is inlined as fixed Tailwind classes so each part has its own cva.
lifeiscontent
force-pushed
the
align/breadcrumb
branch
from
8149e85 to
39a65aa
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 #121.
What the spec says
From the designer's comment on #121:
Always the same (baked in): horizontal layout, separators and gap between items, last-item non-interactive + visually distinct (
text-primary), inline-flex alignment, font style, focus ring on interactive items.Depends / adjustable (caller-controlled): number of items, labels, separator style (slash vs chevron — passed as
childrenonBreadcrumbSeparator), navigation targets, truncation strategy, whether icons appear alongside labels.Architecture changes
ui/breadcrumb/variants.ts
crumbItemVariants(was a bare string literal inBreadcrumbItem).crumbListVariants(was a bare string literal insideBreadcrumb's<ol>).crumbSeparatorVariants— includes--node-size:0.875rem,[&>svg]:size-(--node-size), andrtl:[&>svg]:-scale-x-100so the separator sizes and RTL-mirrors any icon child without baking aclassNameon it.crumbVariants({ interactive: false })soBreadcrumbPage'stext-primarylives in the cva, not in acx()call in the component file.cx(crumbVariants(...), crumbTriggerVariants(...))composition inBreadcrumbTriggerwith a single self-containedcrumbTriggerVariantsthat includes all the base pill styles plus open-state shifts. Also sets[--node-size:0.875rem]for icon children.cximport from component files; components now call a singlexVariants(...).components/breadcrumb/variants.ts (new file)
crumbDropdownItemVariants— moves the inlineclassNamestring out ofBreadcrumbDropdownItem.crumbDropdownPopupVariants— moves theclassNameoffMenu.PopupinBreadcrumbDropdown.crumbMenuTriggerIconSlotVariants— named slot variant for the leading icon inBreadcrumbMenuTrigger(16 px via--node-size:1rem,[&>svg]sizing), replacing the anonymous<span className="flex size-4 ...">wrapper.crumbMenuTriggerIndicatorVariants— trailing chevron classes, moved out of JSX into cva.components/breadcrumb/breadcrumb-dropdown.tsx
<Ellipsis />in<NodeSlot>so the icon is sized by--node-size(inherited from the trigger's cva) rather than a hardcodedclassName="size-3.5".components/breadcrumb/breadcrumb.stories.tsx
className="size-4"from icon passed toBreadcrumbMenuTrigger— the slot variant now handles sizing, so no className is needed on the icon element.Notes
Needs design approval from @bhaveshraja before merge, per the team's review process.