Toolbar: elevation + density as orthogonal axes#97
Conversation
|
📚 Storybook preview: https://pr-97-propel-storybook.vamsi-906.workers.dev |
There was a problem hiding this comment.
Pull request overview
This PR decouples Toolbar hit-target density from placement by introducing an explicit density override prop, enabling combinations like Figma’s “fixed + compact” (a flat topbar with 24px targets) while preserving existing default behavior when density is omitted.
Changes:
- Add optional
density?: "compact" | "comfortable"toToolbar, defaulting toDENSITY_BY_VARIANT[variant]when not provided. - Update inline docs/comments to clarify default density behavior and the override.
- Add a
FixedCompactStorybook story demonstratingvariant="topbar"withdensity="compact".
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/propel/src/components/toolbar/index.tsx | Adds density prop and uses it to override the variant-derived density passed via context. |
| packages/propel/src/components/toolbar/toolbar.stories.tsx | Documents default density behavior and adds a FixedCompact story demonstrating the new override axis. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lifeiscontent
changed the title
|
The approval above was for the earlier version that added density as an explicit override. I've since reworked it into the shape we talked about: instead of layering a density override on top of variant, it splits variant (floater/topbar/bottom-bar, two of which rendered identically) into two separate required axes, elevation (raised card vs flat) and density (compact vs comfortable). That drops the DENSITY_BY_VARIANT derivation, and "fixed + compact" becomes just elevation="flat" density="compact". It's a bigger, breaking change than what you approved, so I'd rather get a fresh look than ride the old approval. Re-requesting review. |
lifeiscontent
force-pushed
the
feat/toolbar-density
branch
from
5ae6997 to
8b4fdba
Compare
Add an optional density prop to Toolbar that overrides the variant-derived default. When omitted, density still falls back to DENSITY_BY_VARIANT[variant] (floater -> compact, topbar/bottom-bar -> comfortable), so existing usage is unchanged. This unblocks Figma's flat fixed + compact bar: a topbar forced to compact (24px) without the floater surface. Adds a FixedCompact story for that case.
Add a DensityOverride story that renders a flat topbar forced to density="compact" and asserts its controls measure 24px (size-6) rather than the comfortable 28px default -> the override is now covered.
Replace the placement-named `variant` (floater/topbar/bottom-bar) with two axes named for what they actually express: - `elevation`: `raised` draws its own card (border + shadow) so it hovers over content; `flat` draws no surface and sits flush in a host bar. topbar and bottom-bar rendered identically, so they fold into one `flat`. - `density`: `compact` (24px hit targets) vs `comfortable` (28px). Both are required props, so the variant->density derivation table and the `density ?? ...` fallback are gone. "Fixed + compact" stops being a special case — it's just `elevation="flat" density="compact"`. Placement is the consumer's job, not the primitive's, so it no longer lives on the component. Also drop the now-dead `defaultVariants` on the internal item/dropdown recipes: density always arrives through context. Breaking: `variant` is removed. floater -> elevation="raised" density="compact"; topbar/bottom-bar -> elevation="flat" density="comfortable".
lifeiscontent
force-pushed
the
feat/toolbar-density
branch
from
8b4fdba to
73e3e67
Compare
Export ToolbarDensity for parity with ToolbarElevation so consumers can reference the allowed density values. The bottom-bar formatting toolbar (Figma 2842-3905) is the fixed + compact bar, but it was rendered with density="comfortable". Set it to compact in both the live story and the recipe source snippet.
The toolbar's
variant(floater / topbar / bottom-bar) was doing three jobs at once: it set the surface, derived the density, and named a placement. Two of its values (topbar,bottom-bar) rendered identically, and density was hard-derived from it throughToolbarDensityContext, which is what blocked Figma's flat "fixed + compact" bar (node 2842-3905): a non-floating surface that still wants the tight 24px floater density.This splits
variantinto the two axes it was really expressing, each named for what it does:elevation:raiseddraws its own card (border + shadow, hovers over content) vsflat(no surface, sits flush in a host bar). The oldtopbar/bottom-barwere visually identical, so they collapse into oneflat.density:compact(24px hit targets) vscomfortable(28px).Both are required props, so the
DENSITY_BY_VARIANTtable and the override fallback both disappear. "Fixed + compact" stops being a special case, it's justelevation="flat" density="compact". Placement (where the bar lives on the page) is the consumer's concern, so it no longer rides on the component.Also drops the now-dead
defaultVariantson the internal item/dropdown recipes; density always arrives through context.Breaking change
variantis removed:variant="floater"becomeselevation="raised" density="compact"variant="topbar"/variant="bottom-bar"becomeelevation="flat" density="comfortable"For design review
This intentionally restructures Figma's single placement-based
variantaxis into two orthogonal axes. Putting it up so the surface/density matrix is visible against the designs, happy to pull it back toward the placement model if that's the preferred shape.Stories
ElevationsandDensitiesare args-driven single-axis showcases;FlatCompactdemos the orthogonal combo against the Figma node;DensityDrivesControlSizeis the browser wiring check.The overflow "..." control from #59 is still deferred pending a UX decision.
Refs #59