Align progress to architecture goals#190
Open
lifeiscontent wants to merge 3 commits into
Open
Conversation
|
📚 Storybook preview: https://pr-190-propel-storybook.vamsi-906.workers.dev |
The designer's spec (issue #145) identified two adjustable axes that the component didn't expose: color/sentiment and indeterminate mode. Both are now first-class required props. Spec alignment: - Always the same: horizontal bar shape, track bg, fill direction, border-radius, track height per magnitude, smooth transition, fill capped at 100% — all baked in via cva base classes. - Depends (adjustable): value, indeterminate mode, tone (brand/success/warning/danger), magnitude, and showValue — all exposed as props (tone and magnitude required, no defaultVariants). Architecture changes: - variants.ts: add `tone` variant to progressIndicatorVariants (bg-*-primary), progressValueVariants (text-*-primary), and ringVariants ([--progress-fill:var(--bg-*-primary)]). The circle's arc now reads --progress-fill so all color logic stays in cva. - Indeterminate: add progress-indeterminate @Keyframes to animations.css; indicator targets data-indeterminate: to animate a sliding fill and suppress the width transition. - ProgressIndicator/ProgressValue/ProgressCircle: tone is now a required prop (no defaultVariants). - components/progress: thread tone through; value widened to number | null for indeterminate support on the linear variant. - Stories: add Tones and Indeterminate stories; update all render functions to pass explicit tone.
The circular ring baked its whole SVG (root > svg > track circle + arc circle) into one ProgressCircle part with inline class strings, so the ring had no composable anatomy and carried styling outside cva. Split it into single-element parts mirroring the linear bar: ProgressCircle (the styled Progress.Root, owns the box magnitude + progressbar a11y), ProgressCircleSvg (the aria-hidden viewport), ProgressCircleTrack (the subtle full ring), and ProgressCircleIndicator (the toned arc; tone drives the stroke). Each renders exactly one element and pulls every class into cva in variants.ts; the geometry (radius, dash offset) stays in the components tier as value-model SVG attributes. The components Progress now composes these parts instead of delegating to a monolithic ring. Make the percentage readout neutral (text-secondary, matching the label) instead of toned: amber-800/green readouts on a neutral surface fail WCAG AA (4.4:1), and the spec puts the semantic color on the fill, not the number. Drop the now-unused tone prop from ProgressValue. Pass the required tone to the toast's progress bar.
Rename each cva variant fn to exactly match the part that consumes it: - rootVariants → progressVariants (Progress root) - trackVariants → progressTrackVariants (ProgressTrack) - circleVariants → progressCircleVariants (ProgressCircle) - circleSvgVariants → progressCircleSvgVariants (ProgressCircleSvg) - circleTrackVariants → progressCircleTrackVariants (ProgressCircleTrack) - circleIndicatorVariants → progressCircleIndicatorVariants (ProgressCircleIndicator)
lifeiscontent
force-pushed
the
align/progress
branch
from
e00bfa2 to
c66caf5
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 #145
What the designer's spec says
From the issue comment, the progress component has two categories of properties:
Always the same (baked in, not exposed as props): horizontal bar shape, track bg color (
layer-3-selected), fill direction left-to-right, border-radius on track and fill bar, track height per size, smooth transition on value change, fill capped at 100%.Depends / adjustable (now exposed as props):
value(0–100 or null for indeterminate),tone(brand / success / warning / danger),magnitude(sm / md), label text (viaProgressLabelcomposition), whether the percentage label is shown (showValue).What changed
New
toneprop (required, no defaultVariants)The indicator fill was hardcoded to
bg-accent-primary. Sentiment color is a first-class adjustable axis per the spec. All color logic now lives invariants.ts:progressIndicatorVariants:tonevariant →bg-accent-primary / bg-success-primary / bg-warning-primary / bg-danger-primaryprogressValueVariants: matchingtext-*-primaryso the percentage stays in the same hue familyringVariants: sets--progress-fill: var(--bg-*-primary)per tone; the SVG arc reads[stroke:var(--progress-fill)]instead of a hardcoded arbitrary valueProgressIndicator,ProgressValue, andProgressCirclenow requiretone. Thecomponents/progress/Progressthreads it through as a required prop at the composition level too.Indeterminate mode
The spec lists indeterminate as an adjustable axis. Base UI's
Progress.Rootalready supportsvalue={null}and setsdata-indeterminateon the indicator. Aprogress-indeterminatekeyframe animation was added toanimations.css; the indicator targetsdata-indeterminate:to animate a sliding 1/3-width fill and suppress the deterministic width transition.components/progress/Progressnow acceptsvalue: number | nullfor the linear variant. The circular variant falls back to0for null since its arc geometry always needs a number.Stories
Added
Tonesstory showing all four sentiments side-by-side, andIndeterminatestory. Allrender-function stories pass explicittone(they bypass the meta args, so they need it spelled out).Architecture checklist
defaultVariants—toneis requiredclassNameorstyleprops on any componentcvainvariants.ts; nocxin.tsxfilestoneis a cva variant not aRecord<Tone, string>inset-inline-start) used in the keyframe for RTL compatibilityNotes
Needs design approval from @bhaveshraja before merging — the
tonenames match the existing token vocabulary (dangernoterror, consistent with badge/button) rather than the designer's natural language in the issue, worth confirming this is aligned with the Figma component naming.