feat(ui): add ProgressBar component - #1983
MartinS-git wants to merge 6 commits into
Conversation
Adds a ProgressBar with determinate (clamped value 0-100) and indeterminate busy modes. The busy indicator keyframe lives in a component-local progressbar.css (imported via global.css layer and the component), matching the Juno CSS convention for animations. Accessibility: role="progressbar" with aria-valuemin/max; busy mode omits aria-valuenow to signal an unknown value, and the value transition is disabled under prefers-reduced-motion (WCAG 2.3.3). Signed-off-by: MartinS-git <info@eyepic.de>
…h mode enum Signed-off-by: MartinS-git <info@eyepic.de>
Remove the flat hold keyframes at the start and end of the busy animation so the indicator moves continuously through both turning points instead of pausing just before 0 and 100. Drive the simulated mode from JS with randomized per-step delays that park near the end, and trim the stories down to Half, Full, Busy and Simulated. Signed-off-by: MartinS-git <info@eyepic.de>
🦋 Changeset detectedLatest commit: c6ea72a The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
There was a problem hiding this comment.
🟡 Changes recommended
Address the outstanding component behavior, accessibility, state-reset, and test-coverage issues.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a themed ProgressBar component with determinate, busy, and simulated modes.
Changes:
- Implements ARIA attributes, clamping, transitions, and simulated progress.
- Adds styling, theme tokens, exports, tests, and Storybook stories.
- Includes a minor-release changeset.
File summaries
| File | Description |
|---|---|
packages/ui-components/src/theme.css |
Adds theme definitions. |
packages/ui-components/src/index.ts |
Adds public exports. |
packages/ui-components/src/global.css |
Registers global styles and theme tokens. |
packages/ui-components/src/components/ProgressBar/ProgressBar.test.tsx |
Adds component tests. |
packages/ui-components/src/components/ProgressBar/ProgressBar.stories.tsx |
Adds Storybook stories. |
packages/ui-components/src/components/ProgressBar/progressbar.css |
Defines busy animation keyframes. |
packages/ui-components/src/components/ProgressBar/ProgressBar.component.tsx |
Implements the component and contains the outstanding review findings. |
packages/ui-components/src/components/ProgressBar/index.ts |
Adds component exports. |
.changeset/progressbar-component.md |
Adds release metadata. |
Review details
Suppressed comments (5)
packages/ui-components/src/components/ProgressBar/ProgressBar.component.tsx:96
- The conditional mount prevents the declared width transition from running when progress starts at 0 or is reset to 0: React inserts/removes the fill already at its final width instead of changing an existing element's width. This breaks the advertised smooth animation for common 0→positive and positive→0 updates; keep a 0%-width fill mounted and only apply the minimum visible width when the value is positive.
clampedValue > 0 && (
packages/ui-components/src/components/ProgressBar/ProgressBar.component.tsx:99
- For small positive values,
minWidthmakes the visual fill larger thanclampedValue: for example,value={0.1}renders a 0.5rem fill whilearia-valuenowremains 0.1. That contradicts the documented percentage scaling; remove the minimum width or explicitly define a separate, intentional minimum-progress behavior.
style={{ width: `${clampedValue}%`, minWidth: "0.5rem" }}
packages/ui-components/src/components/ProgressBar/ProgressBar.component.tsx:83
...propsis spread after the generated accessibility attributes, so callers can override the component's controlled semantics (for example,mode="busy"witharia-valuenow={50}still rendersaria-valuenow, androle/the min/max attributes can also be replaced). This violates the mode/ARIA contract; spread consumer props before these generated attributes or omit the controlled attributes from the public props.
{...props}
packages/ui-components/src/components/ProgressBar/ProgressBar.component.tsx:64
- React preserves
simulatedWidthwhenmodechanges. After a run reaches 95%, switching to another mode and back tosimulatedrenders the old parked width first and then schedules the sequence from 40%, causing a backward jump instead of starting a new simulation. Reset/reinitialize the simulated state whenever entering this mode and cover the mode transition.
React.useEffect(() => {
if (mode !== "simulated") return
if (window.matchMedia?.("(prefers-reduced-motion: reduce)").matches) {
setSimulatedWidth(simulatedSteps[simulatedSteps.length - 1])
packages/ui-components/src/components/ProgressBar/ProgressBar.component.tsx:65
- The reduced-motion branch is a new user-visible behavior, but the tests only exercise the normal timer path; no test makes
matchMedia("(prefers-reduced-motion: reduce)").matchestrue. Add a deterministic test that asserts the simulated fill is parked at 95% immediately and that the timer sequence is skipped.
if (window.matchMedia?.("(prefers-reduced-motion: reduce)").matches) {
setSimulatedWidth(simulatedSteps[simulatedSteps.length - 1])
return
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ndings
Move the busy indicator's keyframe animation from an inline style into a
.juno-progressbar-busy-fill class in the component-local progressbar.css,
consuming the existing juno-progress-busy keyframe by name (matching the
Juno convention, e.g. DateTimePicker's fpFadeInDown).
Also bundle the approved review fixes:
- always render the determinate fill (width 0% at value 0) instead of
gating it behind value > 0
- drop the minWidth from the determinate fill
- spread {...props} before role/aria-* so consumer props cannot override
the ARIA semantics
- reset simulatedWidth on mode change to avoid a stale value
On prefers-reduced-motion the busy animation is intentionally kept running:
in busy mode the bar is indeterminate (no aria-valuenow), so the gentle
1.1s loop is the only signal that work is in progress. The determinate
transition and the simulated animation still honor the preference.
Signed-off-by: MartinS-git <info@eyepic.de>
|
All five review findings are addressed in the current commit (4dc8cb5):
|
Signed-off-by: MartinS-git <info@eyepic.de>
| React.useEffect(() => { | ||
| if (mode !== "simulated") return | ||
| setSimulatedWidth(0) | ||
| if (window.matchMedia?.("(prefers-reduced-motion: reduce)").matches) { |
There was a problem hiding this comment.
window.matchMedia?.('...') will return undefined in test environments (JSDOM doesn't implement matchMedia), and then .matches on undefined throws a TypeError. The fix is to extend the optional chain one step further:
window.matchMedia?.('(prefers-reduced-motion: reduce)')?.matches
The extra ?. before .matches means the whole expression safely evaluates to undefined rather than throwing when matchMedia isn't available.
| className = "", | ||
| ...props | ||
| }: ProgressBarProps): ReactNode => { | ||
| const clampedValue = Math.min(100, Math.max(0, value)) |
There was a problem hiding this comment.
Worth guarding against NaN here. If a consumer passes something like value={bytesReceived / totalBytes} before totalBytes is known, the result is NaN. Math.min and Math.max both propagate NaN unchanged, so clampedValue ends up as NaN, which gets serialised to the string "NaN" on aria-valuenow – an invalid value that screen readers can't interpret. The fill div also disappears because NaN > 0 is false.
A simple guard before the clamp handles this cleanly:
const clampedValue = Math.min(100, Math.max(0, Number.isFinite(value) ? value : 0))
| role="progressbar" | ||
| aria-valuenow={indeterminate ? undefined : clampedValue} | ||
| aria-valuemin={0} | ||
| aria-valuemax={100} |
There was a problem hiding this comment.
In busy and simulated modes, aria-valuenow is correctly omitted (line 83), but aria-valuemin and aria-valuemax are still always rendered. Some assistive technologies, when they see a numeric range without a current value, will announce something like "0 of 100" rather than treating the bar as indeterminate, which is misleading to the user. The fix is to gate all three range attributes together:
aria-valuenow={indeterminate ? undefined : clampedValue}
aria-valuemin={indeterminate ? undefined : 0}
aria-valuemax={indeterminate ? undefined : 100}
| let elapsed = 2700 | ||
| simulatedSteps.forEach((target) => { | ||
| timers.push(setTimeout(() => setSimulatedWidth(target), elapsed)) | ||
| elapsed += 4050 + Math.random() * 5400 |
There was a problem hiding this comment.
Math.random() is called inside the effect, which causes a subtle difference between development and production. React 18 runs effects twice in StrictMode (dev only) to help catch side effects — the first run's timers are immediately cleaned up and discarded, and the second run gets a fresh set of random numbers. So the animation timing you see while developing is different from what runs in production, which makes it hard to verify the pacing visually.
Moving the delay calculation out of the effect and into a useMemo (or a ref set on first render) means both runs use the same values:
const delays = React.useMemo(() => simulatedSteps.map((_, i) => 2700 + i * (4050 + Math.random() * 5400)), [])
| timers.push(setTimeout(() => setSimulatedWidth(target), elapsed)) | ||
| elapsed += 4050 + Math.random() * 5400 | ||
| }) | ||
| return () => timers.forEach(clearTimeout) |
There was a problem hiding this comment.
Minor one: the effect cleanup clears the pending timers, but simulatedWidth state isn't reset until the effect re-runs and calls setSimulatedWidth(0) on line 63. This means there's a brief window – one render frame - where the bar could flash its previous width if mode switches away from and back to simulated. Adding the reset to the cleanup too would make the transition cleaner:
return () => {
timers.forEach(clearTimeout)
setSimulatedWidth(0)
}
| --color-required-bg: var(--color-accent); | ||
| /* DT Spinner */ | ||
| --color-spinner-primary: var(--color-accent); | ||
| /* DT Syntax Highlighting */ |
There was a problem hiding this comment.
Looks like the /* DT Syntax Highlighting */ section comment got accidentally replaced when the ProgressBar block was inserted above it. The --color-syntax-highlight-*-variables are now missing their header, inconsistent with every other section in the file. global.css has the same insertion and preserves the comment correctly there. Just needs the comment added back:
/* DT ProgressBar */
--color-progressbar-fill: var(--color-accent);
--color-progressbar-border: var(--color-text-light);
/* DT Syntax Highlighting */
--color-syntax-highlight-base00: ...
franzheidl
left a comment
franzheidl
left a comment
There was a problem hiding this comment.
accidentally hit "Approve" earlier, I think the changes requested make sense :)
Summary
Adds a new
ProgressBarcomponent to@cloudoperators/juno-ui-componentswith amodeprop offering three modes:determinate: fills the track to a clampedvalue(0-100) with an easedwidthtransition so value jumps animate smoothly.busy: an animated indeterminate indicator (continuous CSS keyframe loop, no plateaus at the turning points).simulated: a JS-driven fake self-running progress that advances through steps separated by randomized delays and parks near the end (~95%), for when the final amount of incoming data is unknown.Accessibility
role="progressbar"witharia-valuemin/aria-valuemax.determinateexposesaria-valuenow;busyandsimulatedomit it to signal an unknown value to assistive technology.prefers-reduced-motion: reduce(WCAG 2.3.3) where the motion is decorative or long-running: the determinatewidthtransition is disabled and the simulated animation parks statically at its end value.busyanimation is intentionally kept running under reduced motion. In that indeterminate state (noaria-valuenow) the gentle 1.1s loop is the only signal that work is in progress; freezing it would leave a static sliver that reads as "stuck". The motion is deliberately low-frequency and non-flashing, staying within the reduced-motion guidance for essential, non-decorative status feedback.Implementation
The
busykeyframe animation lives in a.juno-progressbar-busy-fillclass in the component-localprogressbar.css(consuming thejuno-progress-busykeyframe by name) rather than as an inline style, matching the Juno CSS convention. The determinate fill is always rendered (atwidth: 0%whenvalueis 0), and the simulated fill resets on mode change.Tests
20 unit tests covering rendering, role/ARIA attributes, value clamping, all three modes, the simulated timer advancing to its parked value, and the reduced-motion parking behavior.
Storybook
Stories:
Half,Full,Busy,Simulated. The simulated animation can be replayed via Storybook's built-in Remount (↻) toolbar button.Changeset
minorbump, included.