Skip to content

[WEB-8163] fix(progress): tint value readout to fill tone + production-readiness hardening#232

Open
codingwolf-at wants to merge 6 commits into
mainfrom
WEB-8163/progress-design-docs
Open

[WEB-8163] fix(progress): tint value readout to fill tone + production-readiness hardening#232
codingwolf-at wants to merge 6 commits into
mainfrom
WEB-8163/progress-design-docs

Conversation

@codingwolf-at

@codingwolf-at codingwolf-at commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Description

Design-system finalization pass on the Progress family (LinearProgress + CircularProgress).

Design fix

  • Percentage readout now tints to the fill tone (per Figma text/accent/primary).
    LinearProgressValue gains a tone axis mapping to text-{accent,success,warning,danger}-primary;
    the ready-made and element stories pass it through. Previously a fixed neutral gray.

Audit hardening

  • Harden the root graft (both components): omit render from the public
    Props and spread {...props} before render. Circular previously spread after
    render, letting a consumer-supplied render clobber the ring element.
  • aria-label optional on LinearProgress: it's no longer required when a visible
    label is set (the label names the bar via aria-labelledby). Was forcing a
    visible/accessible name mismatch.
  • Clamp coverage: added a ClampsOutOfRange play test asserting out-of-range
    value clamps to [0, max] via aria-valuenow.

Cleanup

  • Removed redundant Props for {@link X} JSDoc from circular element parts.
  • Added LinearProgressLabel to the component story subcomponents (was missing from docs).

Test

  • vp check and vp test green.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Improvement (change that would cause existing functionality to not work as expected)
  • Code refactoring
  • Performance improvements
  • Documentation update

Screenshots and Media (if applicable)

Screenshot 2026-07-15 at 8 11 34 PM Screenshot 2026-07-15 at 8 11 51 PM Screenshot 2026-07-15 at 8 12 25 PM Screenshot 2026-07-15 at 8 12 36 PM

Figma URL

…lue readout

- Updated `LinearProgressValue` to accept a `tone` prop, allowing the percentage readout to match the fill's tone as per design specifications.
- Modified related stories to demonstrate the new tone functionality, ensuring consistent visual representation across different progress states.
- Adjusted the `linearProgressValueVariants` to include tone variants, improving the semantic color application on the readout.
…ponent

- Introduced a new story, `ClampsOutOfRange`, to demonstrate the clamping of values exceeding the maximum limit, ensuring `aria-valuenow` reflects the clamped value.
- Updated `CircularProgressProps` to exclude the `render` prop, streamlining the component's API.
- Enhanced LinearProgress component to support an optional `aria-label` when a `label` is provided, improving accessibility.
- Removed unnecessary prop documentation comments in circular progress elements for cleaner code.
@makeplane

makeplane Bot commented Jul 15, 2026

Copy link
Copy Markdown

Linked to Plane Work Item(s)

This comment was auto-generated by Plane

@github-actions

Copy link
Copy Markdown

📚 Storybook preview: https://pr-232-propel-storybook.vamsi-906.workers.dev

@anmolsinghbhatia

Copy link
Copy Markdown

@codingwolf-at The build is failing. Please take a look.

@@ -27,8 +30,8 @@ export type LinearProgressProps = Omit<BaseProgress.Root.Props, "className" | "s
showValue?: boolean;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aria-label is now optional while label is optional too, so an unnamed progressbar type-checks and renders. Could the props require at least one accessible name (label or aria-label) to preserve the a11y invariant?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. LinearProgressProps is now a discriminated union — label or aria-label is required, so an unnamed progressbar is a compile error:

export type LinearProgressProps = LinearProgressOwnProps &
  ({ label: string; "aria-label"?: string } | { label?: undefined; "aria-label": string });

Labeled → named by the visible label; no label → aria-label required. (Same pattern as Badge.)

export function CircularProgress({ value, magnitude, tone, ...props }: CircularProgressProps) {
const { box, radius } = RING_GEOMETRY[magnitude];
const circumference = 2 * Math.PI * radius;
const max = props.max ?? 100;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

min still forwards from BaseProgress.Root.Props, but clamping and the arc fraction assume a zero minimum. For min={20} max={100} value={60}, the ring paints 60% instead of 50%. Could we derive both bounds and the fraction from min/max (or omit min from this API)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — the arc fraction and clamp now derive from both bounds:

const min = props.min ?? 0;
const span = max - min;
const clampedValue = value == null ? null : Math.min(Math.max(value, min), max);
const fraction = clampedValue == null ? 0.25 : span > 0 ? (clampedValue - min) / span : 0;

min=20 max=100 value=60 now paints (60−20)/80 = 50%, and the clamp floors at min. span > 0 guards min===max.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CircularProgress now clamps out-of-range values, but LinearProgress still passes them through. value={150} produces aria-valuenow="150" with aria-valuemax="100" and a 150% label. Could we apply the same clamp before passing the value to Base UI?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — LinearProgress now clamps to [min, max] before Base UI derives the fill, aria-valuenow, and the % label, matching CircularProgress:

const clampedValue = value == null ? null : Math.min(Math.max(value, min), max);

alue={150} now reads aria-valuenow="100" and a 100% label instead of overflowing.

@anmolsinghbhatia

Copy link
Copy Markdown

@codingwolf-at Build is failing. Please take a look into this.

- Removed the `tone` prop from `LinearProgressValue`, simplifying the component's API and ensuring the percentage readout is a neutral number.
- Updated the `linearProgressValueVariants` to reflect the change, aligning the styling with the design specifications that emphasize a neutral readout.
- Adjusted related stories to remove tone references, ensuring consistent representation across different progress states.
@codingwolf-at

Copy link
Copy Markdown
Contributor Author

@codingwolf-at Build is failing. Please take a look into this.

Build failed on an axe color-contrast violation in LinearProgress > Tones:
.text-warning-primary — 4.49 (fg #bb4d00, bg #f2f2f3, 12px). Expected 4.5:1

The change tinted the % readout to the fill tone. That's fine for brand/danger, but warning (amber) and success (green) fail AA as 12px text on a neutral surface — warning lands at 4.49, just under 4.5.

Reverting the readout to neutral text-secondary: no AA-safe way to tint amber/green there, and the semantic color already lives on the bar fill. Contrast passes again.

…ility

- Changed the type definition for `LinearProgressProps` to enforce a union of `label` and `aria-label`, ensuring that one of them is always provided for accessibility.
- Updated the story type to be directly tied to `LinearProgress`, enhancing type-checking for individual stories.
- Added documentation comments to clarify the new prop requirements and improve code readability.
…lue calculation

- Introduced `min` prop to allow for customizable lower bounds in value clamping, ensuring accurate representation of progress.
- Updated the clamping logic to derive the fraction based on both `min` and `max`, improving the component's flexibility and correctness.
- Enhanced comments for clarity on the clamping behavior and its impact on accessibility attributes.
…ogress representation

- Introduced `min` and `max` props to allow customizable bounds for value clamping, ensuring that the displayed progress does not exceed defined limits.
- Updated clamping logic to prevent discrepancies between the displayed value and accessibility attributes, enhancing overall component reliability.
- Improved comments for clarity on the clamping behavior and its implications for accessibility.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants