Repository navigation
docs(spec): ProgressBar records its shipped contract as current - #9135
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
the
needs:spec-owner-review
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval waits on an engineering-owner decision. This head makes component:ProgressBar current, including DEC-1 and FR1–FR9, and no current record states those requirements yet. An engineering owner records the decision by commenting /approve-spec with the full SHA of the head they approve (the author may do this as an engineering owner). Separately, these record lines need fixing either way:
- The Variant row says "Closed union; rejected by types", but
ProgressBarVariantMapis exported for module augmentation, and an added variant paints no fill until a theme styles it. - FR9 requires caller data props to land "after the component's attributes", which is exactly the
data-variantoverride that FR8 calls a gap; one of the two has to change. - The "Builder impact" row describes the
data-variantoverride backwards. - The FR4 verification row says a track that is not 8px fails, but the "renders at fixed 8px track height" test only checks that the bar renders.
- FR6 says the indeterminate slide mirrors under right-to-left, but it follows any
dir="rtl"ancestor rather than the bar's own direction, unlike the determinate fill.
[Automated review]
| | Value and range | finite `value` within `0`–`max` | Completed amount. | Determinate | 0 of 100 | caller | Stable | Clamped to `[0, max]`; a non-finite value or `max` counts as 0. | | ||
| | Label | string, visible or visually hidden | The bar's accessible name. | Always | visible | caller | Stable | — | | ||
| | Value text | hidden; shown; custom formatter | Optional visible value beside the label, also the `aria-valuetext`. | Determinate | hidden; rounded percent | `component:ProgressBar` | Stable | Ignored when indeterminate. | | ||
| | Variant | `accent`, `success`, `warning`, `error`, `neutral` | Fill color. | Always | `accent` | `component:ProgressBar` | Stable | Closed union; rejected by types. | |
There was a problem hiding this comment.
ProgressBarVariant is keyof ProgressBarVariantMap, which index.ts exports for module augmentation, so this axis is not closed. An added variant matches nothing in variantStyles/markOnFillStyles, so its fill paints no colour until a theme styles it, which conflicts with FR1 and component-theming-surface INV14. Consider recording the extension point plus a Current-state impact bullet for the missing baseline.
| | FR8 | The root carries `progress-bar` reflecting `variant`, the track `progress-bar-track`, the fill `progress-bar-fill` reflecting the resolved fill variant, and each mark `progress-bar-mark` reflecting variant and placement; the label and value text carry no target. Reflected state stays component-owned (`architecture:public-component-api` INV5). Implementation gap: a caller `data-variant` replaces the root's reflected variant (Current-state impact). | Current source and tests | | ||
| | FR9 | `ref` and `data-testid` reach the root; supported DOM, data, ARIA, and event props reach the root after the component's attributes; `xstyle`, `className`, and `style` compose after component styles. | Current source and tests; `architecture:public-component-api` INV5–INV8 | |
There was a problem hiding this comment.
FR9's "reach the root after the component's attributes" is the ordering that lets a caller data-variant replace the reflected one, which FR8 calls a gap. As written, no implementation satisfies both; consider excluding reflected theme attributes from FR9.
| | Public contract | `label`, `value`, `max`, `isLabelHidden`, `hasValueLabel`, `formatValueLabel`, `variant`, `isIndeterminate`, `marks`, `isDisabled`, root `BaseProps`, and a root `ref`. | | ||
| | Behavior | A labelled `progressbar` whose fill shows the clamped value against a muted track, or slides while indeterminate, with optional value text and labelled target marks. | | ||
| | End-user impact | People see how far an operation has gone, and assistive technology reads the name and value; marks reveal their labels on hover or focus. | | ||
| | Builder impact | A builder's `data-variant` is replaced by the reflected variant today (FR8 gap); otherwise none. | |
There was a problem hiding this comment.
This is backwards: {...rest} spreads after themeProps, so the builder's data-variant replaces the reflected variant, as Current-state impact already says.
| | FR1, AR1, AR3 | Stable visual baselines `core-progressbar--default` (neutral) and `core-progressbar--with-target-mark` (probe) | Default; probe theme with a mark | A changed fill or track comes back changed for review. Gap: no real-browser contrast check across shipped themes and color modes proves the standalone distinction (OQ1 holds the treatment). | `audit:ProgressBar/visual` | | ||
| | FR2 | Public type and consumer-doc review | Current props | Gap: no automated check; a new visual-distinction prop is a review finding. | `audit:ProgressBar/api` | | ||
| | FR3, ORD1, AR2 | `ProgressBar.test.tsx` label, value text, formatter, clamp, zero and NaN tests; `ProgressBar.a11y.test.tsx` and `ProgressBar.a11y.chromium.spec.ts` states | Determinate; formatted; complete; label hidden | A missing name, a value outside `[0, max]`, a leaked `NaN`, or wrong `aria-valuetext` fails, including at the accessibility tree. Gap: no test checks the default text in a non-English locale. | `audit:ProgressBar/a11y` | | ||
| | FR4 | `ProgressBar.test.tsx` track-height, track-clip, transform, variant, and forced-colors tests; `ProgressBarMotion.a11y.chromium.spec.ts` | Every variant; value changes; RTL region; reduced motion; forced colors | A track that is not 8px or does not clip, a fill sized by width outside forced colors, a lost forced-colors outline, a leading edge off the value or unmirrored in an RTL region, a value change that does not land at once under reduced motion, or per-frame layout while moving fails. Gap: the variant colors have no direct check. | `audit:ProgressBar/visual` | |
There was a problem hiding this comment.
"A track that is not 8px … fails": the renders at fixed 8px track height test (ProgressBar.test.tsx:104-107) only asserts that the progressbar is in the document, and no other test checks the height. Either list this as a gap or make the test assert it.
| | FR3 | The root holds a label row and a track. The label names the track (`role="progressbar"`) through `aria-labelledby`; `isLabelHidden` hides it visually only. When determinate, the track exposes `aria-valuenow` (the clamped value), `aria-valuemin` 0, `aria-valuemax` (`max`), and `aria-valuetext` from `formatValueLabel`, which defaults to the rounded percentage; `hasValueLabel` shows the same text beside the label. A zero or non-finite `max` gives 0% and never renders `NaN`. Astryx-owned value text comes from one catalog message (`architecture:internationalization` INV1a). Implementation gap: the default text is assembled in code (Current-state impact). | Current source and tests | | ||
| | FR4 | The track is 8px high, fully rounded, in `--color-background-muted`, and clips the fill. The determinate fill spans the track and slides along it so its leading edge sits at the clamped percentage, mirrored by the bar's own writing direction, moving over `--duration-medium` when the value changes; under reduced motion a new value lands at once on every pointer (`design:reduced-motion` DR1). The fill paints `--color-accent`, `--color-success`, `--color-warning`, or `--color-error`, or `--color-text-disabled` for `neutral` and disabled bars. In forced colors the fill is sized to the value instead of sliding and keeps a 1px `CanvasText` outline so its shape stays visible (`architecture:theme-tokens` INV11). | Current source and tests | | ||
| | FR5 | `isDisabled` mutes the fill, label, value text, and marks and adds no ARIA state. | Current source and tests | | ||
| | FR6 | `isIndeterminate` removes every value attribute and the value text and slides a 40% fill across the clipped track, mirrored under right-to-left; under reduced motion the loop slows from 1.5s to 3s per pass. Whether the loop stops under reduced motion is open (OQ2). | Current source and tests | |
There was a problem hiding this comment.
The indeterminate slide keys on :is([dir="rtl"] *), so it mirrors under any RTL ancestor, unlike the determinate fill's :dir(rtl) (FR4). In an LTR region of an RTL page it runs against the label. Mark centering has the same issue. Consider stating this and recording it under Current-state impact.
e1fbd16 to
7bd584c
Compare
There was a problem hiding this comment.
Approval waits on an engineering-owner decision: this head makes component:ProgressBar current, including DEC-1 and FR1–FR9, and no current record states those requirements yet. An engineering owner records the decision by commenting /approve-spec with the full SHA of the head they approve, and the author can do this as an engineering owner. The five record lines flagged on the earlier head are fixed; because an approval binds the record text, please fold in these two fixes first:
- FR8: say each mark reflects the resolved fill variant and placement, since a disabled bar's marks reflect
disabledandProgressBar.test.tsxpins that. - FR6 verification row: note that the indeterminate RTL test asserts the
[dir="rtl"]ancestor selector, so it pins the FR6 gap, as the FR7 row already does for marks.
[Automated review]
| | FR5 | `isDisabled` mutes the fill, label, value text, and marks and adds no ARIA state. | Current source and tests | | ||
| | FR6 | `isIndeterminate` removes every value attribute and the value text and slides a 40% fill across the clipped track, mirrored by the bar's own rendered direction (`architecture:internationalization` INV11); under reduced motion the loop slows from 1.5s to 3s per pass. Whether the loop stops under reduced motion is open (OQ2). Implementation gap: the slide, and the centering of marks, mirror under any right-to-left ancestor rather than the bar's own direction, so in a left-to-right region of a right-to-left page they run against the label (Current-state impact). | Current source and tests | | ||
| | FR7 | Each finite mark renders, outside the `progressbar` element, a tick at its clamped position whose tooltip shows its label on hover and focus; marks inside the filled part take the fill's contrasting color, and marks do not render while indeterminate. A focusable mark exposes its label as its accessible name (WCAG 2.2 SC 4.1.2). Implementation gap: marks are focusable `span`s with no role or name, labelled only as a description once the tooltip loads (Current-state impact). | Current source and tests | | ||
| | FR8 | The root carries `progress-bar` reflecting `variant`, the track `progress-bar-track`, the fill `progress-bar-fill` reflecting the resolved fill variant, and each mark `progress-bar-mark` reflecting variant and placement; the label and value text carry no target. Reflected state stays component-owned (`architecture:public-component-api` INV5). Implementation gap: a caller `data-variant` replaces the root's reflected variant (Current-state impact). | Current source and tests | |
There was a problem hiding this comment.
FR8 does not say which variant a mark reflects; shipped marks reflect the resolved fill variant, so a disabled bar's marks carry data-variant="disabled" (ProgressBar.tsx:625-628, pinned by ProgressBar.test.tsx:470-484). Suggest "each mark progress-bar-mark reflecting the resolved fill variant and placement".
| | FR3, ORD1, AR2 | `ProgressBar.test.tsx` label, value text, formatter, clamp, zero and NaN tests; `ProgressBar.a11y.test.tsx` and `ProgressBar.a11y.chromium.spec.ts` states | Determinate; formatted; complete; label hidden | A missing name, a value outside `[0, max]`, a leaked `NaN`, or wrong `aria-valuetext` fails, including at the accessibility tree. Gap: no test checks the default text in a non-English locale. | `audit:ProgressBar/a11y` | | ||
| | FR4 | `ProgressBar.test.tsx` track-clip, transform, variant, and forced-colors tests; `ProgressBarMotion.a11y.chromium.spec.ts` | Every variant; value changes; RTL region; reduced motion; forced colors | A track that does not clip, a fill sized by width outside forced colors, a lost forced-colors outline, a leading edge off the value or unmirrored in an RTL region, a value change that does not land at once under reduced motion, or per-frame layout while moving fails. Gap: the variant colors have no direct check. Gap: the "fixed 8px track height" test only checks that the bar renders, so the track height has no check; no test covers a theme-added variant. | `audit:ProgressBar/visual` | | ||
| | FR5 | `ProgressBar.test.tsx` disabled and disabled-mark tests | Disabled | A missing label or undimmed marks fail. Gap: the muted fill and label colors have no direct check. | `audit:ProgressBar/visual` | | ||
| | FR6 | `ProgressBar.test.tsx` indeterminate suite | Every variant; RTL | Value attributes or value text while indeterminate, a lost label, an unclipped track, or an unmirrored slide fails. Gap: the reduced-motion loop duration has no check. No test covers the slide in a left-to-right region inside a right-to-left page. | `audit:ProgressBar/behavior` | |
There was a problem hiding this comment.
The indeterminate RTL test asserts the ancestor selector (ProgressBar.test.tsx:264 matches :is([dir="rtl"] …)), so it pins the FR6 gap; a fix to the bar's own direction fails it. Worth a "Gap:" note like the FR7 row's.
| | FR1 | Every determinate presentation ProgressBar provides is a sufficient standalone visual for distinguishing completed from remaining progress; external visible text may supplement it but is never required, and supplementary text or graphics never substitute for that distinction. ProgressBar does not make visual correctness depend on caller-declared external content it cannot verify or associate. | `component:ProgressBar/DEC-1`; `spec:AST-002` | | ||
| | FR2 | New public API that adjusts the visual distinction is considered only after the component has a correct base behavior and a stable caller-owned distinction still remains. | `component:ProgressBar/DEC-1`; `spec:AST-002` | | ||
| | FR3 | The root holds a label row and a track. The label names the track (`role="progressbar"`) through `aria-labelledby`; `isLabelHidden` hides it visually only. When determinate, the track exposes `aria-valuenow` (the clamped value), `aria-valuemin` 0, `aria-valuemax` (`max`), and `aria-valuetext` from `formatValueLabel`, which defaults to the rounded percentage; `hasValueLabel` shows the same text beside the label. A zero or non-finite `max` gives 0% and never renders `NaN`. Astryx-owned value text comes from one catalog message (`architecture:internationalization` INV1a). Implementation gap: the default text is assembled in code (Current-state impact). | Current source and tests | | ||
| | FR4 | The track is 8px high, fully rounded, in `--color-background-muted`, and clips the fill. The determinate fill spans the track and slides along it so its leading edge sits at the clamped percentage, mirrored by the bar's own writing direction, moving over `--duration-medium` when the value changes; under reduced motion a new value lands at once on every pointer (`design:reduced-motion` DR1). The fill paints `--color-accent`, `--color-success`, `--color-warning`, or `--color-error`, or `--color-text-disabled` for `neutral` and disabled bars; a theme-added variant has a safe theme-independent fill (`architecture:component-theming-surface` INV14). Implementation gap: a theme-added variant paints no fill or mark color until a theme styles it (Current-state impact). In forced colors the fill is sized to the value instead of sliding and keeps a 1px `CanvasText` outline so its shape stays visible (`architecture:theme-tokens` INV11). | Current source and tests | |
There was a problem hiding this comment.
Minor: marks on the bare track still paint --color-text-primary for a theme-added variant (ProgressBar.tsx:634-636); only the fill and on-fill marks lose colour. "no fill and no on-fill mark color" would be exact.
7bd584c to
2472aec
Compare
There was a problem hiding this comment.
Approval waits on an engineering-owner decision: this head makes component:ProgressBar current, including DEC-1 and FR1–FR9, and no current record states those requirements yet. An engineering owner records the decision by commenting /approve-spec with the full SHA of the head they approve, and the author can do this as an engineering owner. The three record lines flagged on the earlier head are fixed; two optional wording fixes are cheapest to fold in before an approval binds the text:
- Representative states, With marks: "labelled by tooltip" reads as the description-only path that FR7 calls a gap; say the tooltip shows each label (FR7).
- FR4: cite
design:reduced-motionDR2/DEC-3 for "lands at once on every pointer", since DR1 only forbids travel.
[Automated review]
| | With value text | The graphic still satisfies FR1; the text supplements it. | Formatter. | | ||
| | Label hidden | The bar keeps its accessible name. | — | | ||
| | Indeterminate | No value attributes; sliding fill; slower under reduced motion (OQ2). | Variant. | | ||
| | With marks | Ticks outside the `progressbar`, labelled by tooltip. | Mark labels. | |
There was a problem hiding this comment.
"labelled by tooltip" reads as the description-only path FR7 calls a gap (FR7 requires the label as the mark's accessible name). Suggest: "Ticks outside the progressbar; the tooltip shows each label (FR7)."
| | FR1 | Every determinate presentation ProgressBar provides is a sufficient standalone visual for distinguishing completed from remaining progress; external visible text may supplement it but is never required, and supplementary text or graphics never substitute for that distinction. ProgressBar does not make visual correctness depend on caller-declared external content it cannot verify or associate. | `component:ProgressBar/DEC-1`; `spec:AST-002` | | ||
| | FR2 | New public API that adjusts the visual distinction is considered only after the component has a correct base behavior and a stable caller-owned distinction still remains. | `component:ProgressBar/DEC-1`; `spec:AST-002` | | ||
| | FR3 | The root holds a label row and a track. The label names the track (`role="progressbar"`) through `aria-labelledby`; `isLabelHidden` hides it visually only. When determinate, the track exposes `aria-valuenow` (the clamped value), `aria-valuemin` 0, `aria-valuemax` (`max`), and `aria-valuetext` from `formatValueLabel`, which defaults to the rounded percentage; `hasValueLabel` shows the same text beside the label. A zero or non-finite `max` gives 0% and never renders `NaN`. Astryx-owned value text comes from one catalog message (`architecture:internationalization` INV1a). Implementation gap: the default text is assembled in code (Current-state impact). | Current source and tests | | ||
| | FR4 | The track is 8px high, fully rounded, in `--color-background-muted`, and clips the fill. The determinate fill spans the track and slides along it so its leading edge sits at the clamped percentage, mirrored by the bar's own writing direction, moving over `--duration-medium` when the value changes; under reduced motion a new value lands at once on every pointer (`design:reduced-motion` DR1). The fill paints `--color-accent`, `--color-success`, `--color-warning`, or `--color-error`, or `--color-text-disabled` for `neutral` and disabled bars; a theme-added variant has a safe theme-independent fill (`architecture:component-theming-surface` INV14). Implementation gap: a theme-added variant paints no fill and no on-fill mark color until a theme styles it (Current-state impact). In forced colors the fill is sized to the value instead of sliding and keeps a 1px `CanvasText` outline so its shape stays visible (`architecture:theme-tokens` INV11). | Current source and tests | |
There was a problem hiding this comment.
Minor: DR1 only forbids travel; the at-once rule for an in-place value on every pointer is design:reduced-motion DR2 and DEC-3 (its verification row names a determinate ProgressBar's fill). Suggest citing DR2/DEC-3 here.
|
/approve-spec 2472aec |
github-actions
Bot
removed
the
needs:spec-owner-review
Exact decision needed
Promote
component:ProgressBarfromdrafttocurrent, recording ProgressBar's shipped behavior on main today. No behavior, API, or output change.Existing authority searched
component:ProgressBaron main (draft, template 3): DEC-1 (a sufficient standalone visual), its FR1–FR5 consequences, the theming anatomy, and OQ1 (the exact standalone treatment) were re-verified and kept; FR1–FR2 now carry DEC-1 and its API gate. The record is completed to template 7 so value semantics, value text, variants, disabled, indeterminate motion, marks, and caller routing map to it, including the transform-based fill and reduced-motion landing that shipped in the fill-motion change on main.design:reduced-motion,architecture:theme-tokens, andarchitecture:internationalization(current) govern motion, forced colors, and Astryx-owned text.pnpm specs:for packages/core/src/ProgressBar/ProgressBar.tsx: directory-wide architecture records.Decision
packages/core/src/ProgressBar/ProgressBar.spec.md:authority: current,approved_by: cixzhang,approved_at: 2026-10-31,ownersunchanged, template 7, behavioral table headed "Invariant | Basis". DEC-1 is unchanged. New OQ2 asks whether the indeterminate loop stops under reduced motion.Rejected alternatives
None.
Claim scope and non-goals
Covers only ProgressBar as shipped on main. External labels and values stay with the caller; the exact standalone treatment stays open (OQ1); the mark tooltip stays with
component:Tooltip. No new behavior is decided.Evidence and implementation relationship
ProgressBar.a11y.test.tsxandProgressBar.a11y.chromium.spec.ts.ProgressBarMotion.a11y.chromium.spec.ts(leading edge, RTL region, reduced motion, per-frame cost).core-progressbar--default(neutral) andcore-progressbar--with-target-mark(probe).Current-state impact
aria-valuetextare assembled in code, not one catalog message (architecture:internationalizationINV1a).spans with no role or accessible name (WCAG 2.2 SC 4.1.2).architecture:component-theming-surfaceINV14).architecture:internationalizationINV11).data-variantreplaces the root's reflected variant (architecture:public-component-apiINV5).Writing rubric results
#+ digits,/pull/,/issues/).Scope
Validation
pnpm check:knowledge: alignedgit diff --check: clean