fix: design-system audit — motion, interaction states, focus & accessibility#856
Conversation
Compose the shared easing, duration, pressed-scale and focus-ring tokens so components stop hand-rolling their own timings and curves.
Step body letter-spacing and h1-h4 line-heights so they change monotonically across sizes and follow Inter's own tracking curve.
Give numeric columns tabular figures so digits align, alongside the shared row-selection and sticky-header styling that lives in the same two stylesheets.
A slightly faster spinner raises perceived load speed. Recomputed all 8 pole animation-delays to keep the same 1/8-cycle stagger ratio at the new duration, so the sweep still rotates evenly.
Centering used translateX(-50%), which would collide with any transform-based entrance. Move centering to inset+margin-inline:auto (safe since the root is width:fit-content) so transform is free for motion. Add a fade-in for everyone, plus an 8px rise from the anchored edge under prefers-reduced-motion: no-preference. Exit stays instant (no mount controller exists to hold the node past removal).
The error handler swapped src to the fallback with no re-entry guard -- if the fallback URL itself 404s, onError fires again and re-assigns the same failing src, looping requests indefinitely. Track a one-shot ref so the fallback is only ever assigned once; a second failure falls through to showing the alt/broken rendering instead. Both Image and Avatar's photo popped in abruptly with no load transition. Image now tracks load state via a useLayoutEffect that checks node.complete on mount/src-change -- already-decoded (cached or SSR-painted) images stay visible with no fade; genuinely loading images fade in over 150ms on load. Avatar's image only mounts once Base UI's probe reports loaded, so its fade is a one-shot keyframe animation, skipped entirely for cache-fast loads (<100ms between loading and loaded) so a quick load isn't artificially delayed. Reduced motion shows both instantly at full opacity, never invisible.
A display:block heading already fills its container in normal flow; the explicit width:100% only mattered in flex/grid contexts, where it made a heading claim the full row/track and push siblings around. Ellipsis truncation is the only variant that needs an explicit width, and it already declares its own -- removing the base rule loses nothing there. BREAKING (mild): consumers relying on the base width:100% inside flex/ grid parents (e.g. a centered heading, or a heading used as a full-row flex child) will see it shrink to content width. One-line fix on their side: set width:100% via className where the old behavior was intentional.
The action button was the bar's only interactive element but had no hover, press, or focus feedback. Add opacity-step hover/press states and a currentColor focus ring that stays visible against the bar's coloured backgrounds.
…hiding it Both variants hid the shared Tabs.Indicator and painted the active state directly on the trigger, so switching tabs snapped instantly instead of gliding like the default segmented variant. The indicator already tracks the active tab via Base UI's --active-tab-* variables for free -- hiding it threw that away. Restyle the indicator per variant instead: standalone becomes the active chip fill (background + border) gliding between tabs, with the active trigger going transparent so the pill shows through (the same trick the segmented variant already uses); plain becomes a 1px underline sliding along the bottom edge, positioned via the trigger's own --active-tab-height so it lines up with the reserved border-bottom exactly. Both inherit the indicator's existing --rs-duration-moderate/--rs-ease-in-out transition, gated behind reduced motion, with no new media query needed.
Breadcrumb links changed colour instantly on hover while the rest of the library eases. Add a short colour transition, gated behind prefers-reduced-motion, so the hover matches every other interactive element.
Popover -- a larger floating surface than the tooltip -- cast a lighter shadow (--rs-shadow-soft) than the tiny tooltip (--rs-shadow-lifted), so elevation read backwards when both were on screen. Match menu, a peer surface of similar size, which already uses --rs-shadow-lifted.
overflow: scroll forced a permanently visible scrollbar gutter on platforms with classic scrollbars, even when content didn't overflow. Use auto. Also document via JSDoc that SidePanel intentionally has no enter/exit animation -- it's persistent layout chrome like Sidebar, not an overlay.
The scrollbar grew its whole track on hover, reflowing the page each time, and the thumb never showed it was grabbed. Keep the track a constant, invisible width so layout never shifts; draw the thumb thinner at rest and grow it to full width on hover; and deepen its colour one step on hover and again while dragging.
The positioner transitioned top/left/right/bottom and the popup transitioned width/height on every open -- but Base UI only writes inset positioning and drives --popup-width/--popup-height when a PreviewCard.Viewport child triggers its adaptiveOrigin mode. In the common case (no Viewport), Base UI positions purely via transform, so the four inset transitions were dead weight forcing unnecessary transition-list overhead on every reposition. Split into two rulesets: the default path transitions transform + opacity only (compositor-only), while `:has(.viewport)` keeps the inset/size transitions where they're the actual morph mechanism.
A sticky header drew its bottom hairline even at rest, before anything had scrolled beneath it. Drive the hairline from a scroll-linked animation so it fades in over the first bit of scroll, gated behind a real scroll container so the plain table keeps its static separator.
Resting day cells used radius-2 while hover/selected/range-cap states all used radius-5, so sweeping the pointer across the grid made each cell's shape pop between rounded-square and pill. The radius-2 resting state was never actually visible (the cell's background matches the calendar root's), so unify on radius-5 -- zero visible states change, one invisible one gets fixed. Hover/selection highlights stay instant, per the audit's explicit rejection of transitions on direct-manipulation surfaces. Loading hard-swapped the whole table for a differently-sized Skeleton in one frame, flashing on fast loads and jumping the popover height. Keep the table mounted and overlay the skeleton, cross-fading via opacity/visibility (~150ms, instant under reduced motion) so the grid never disappears and the height never moves.
Dimming a link to 80% opacity on hover is a weak signal and makes the text harder to read at the exact moment the user is targeting it. The underline geometry now always exists (text-decoration-color: transparent at rest) so hover is a pure color fade to currentColor -- zero layout shift, works for every Text color variant automatically. Press gets a brief variant-agnostic dim (:active, opacity 0.7) instead of a hardcoded color, since a fixed active color would clobber non-accent variants. Timing moves from a hardcoded 0.2s to --rs-duration-fast.
Proportional digits meant amounts jittered as values changed and misaligned in table columns. Add a CSS module with font-variant-numeric: tabular-nums as the default; className/style still merge through as the opt-out escape hatch.
The floating copy button had no transition of its own duration (it inherited the interactive tier's opacity fade), so its reveal moved at hover-feedback speed instead of a dedicated reveal timing. Re-compose the IconButton base transition and override just the opacity entry with --rs-duration-fast/--rs-ease-out. The :focus-visible reveal rule (added by the earlier focus-indicators pass) is merged into the same selector as the hover reveal for clarity.
Row selection was wired up in data-table and data-view-beta (both set
data-state="selected") but no CSS anywhere styled it -- selecting a
row changed nothing on screen. Add an accent-subtle background for
selected rows (wins over hover/pressed) and a one-step-darker pressed
background for clickable rows, instant (no transition, matching the
house rule for direct-manipulation highlights like menu items).
Virtualized cells painted an opaque background that covered the row's
color entirely -- add `.virtualRow .virtualCell { background: inherit }`
so selection/hover/press show through in virtualized mode, same as the
non-virtualized cells already do.
data-view (no selection support) gets the same pressed-row treatment.
The bare Table primitive had no row-state hooks at all. Table.Row now
accepts an `interactive` prop (cursor + hover + pressed) and responds
to a consumer-set data-state="selected", with cells inheriting the row
background through the opaque .cell rule.
…bled cursor Hovering an off toggle looked identical to the on state -- the control lied about whether it was selected. Give hover a lighter tint and pressed (Base UI's toggled-on state) a deeper, inset-shadowed look in both standalone and group mode; hovering an already-on toggle now deepens it one more step instead of reverting to the hover tint. The content background and inset shadow had no transition at all (only color did), so state changes snapped. Add a fast-tier transition for both. pointer-events: none on disabled toggles suppressed the declared cursor: not-allowed, so disabled toggles gave zero affordance. Restore pointer-events so the cursor shows, with explicit hover neutralizers so a disabled-but-off toggle doesn't pick up hover styling (clicks stay blocked natively via the disabled <button>).
The select positioner painted an opaque square that flashed behind the popup as it scaled, and multi-select options nested a real focusable checkbox inside each row, adding a stray tab stop. Drop the positioner background and use a presentational check indicator; the trigger shows a static up/down caret to signal it opens a menu.
A chip with onClick rendered as a <span onClick>: no keyboard activation, not in the tab order, no focus ring, and cursor: default even though it was clickable. Render a <button type="button"> when onClick is passed and the chip isn't dismissible (a button containing the dismiss button would be invalid nested-interactive HTML, so dismissible chips keep the span). Interactive chips get cursor: pointer, inherit the app font, and a focus-visible accent ring.
The Field error popped in with no transition and replaced the description in one jarring jump, and the DatePicker set an error that was never shown. Cross-fade the error in over the description so the swap is smooth, and render the DatePicker error as reserved-space helper text below the input.
…es in The 2D area thumb was positioned via left/top on every pointermove, forcing layout on each frame of a drag. Drive it from two CSS custom properties (--thumb-x/--thumb-y, unitless 0..1 fractions set from JS) composed with container query units and the existing -50% self-centering transform -- compositor-only, no transition on transform so the drag stays 1:1 with the pointer. Also add touch-action: none / user-select: none to .selectionRoot (the sliders already had this; the 2D area didn't, so dragging on touch also scrolled the page), and a fast opacity transition on the thumb so its first-render reveal fades instead of popping.
.nav-text declared an opacity transition, but the collapsed state set display: none, which can't transition and short-circuited it -- labels popped out of existence while the frame animated its width. Same for any consumer content marked data-collapse-hidden. Replace display:none with opacity + visibility (visibility keeps collapsed content out of the accessibility tree and unfocusable, same intent as display:none, but participates in the transition). Also fix the resize handle's cursor: it only click-toggles collapse, but showed ew-resize, promising a drag that doesn't exist. Use pointer on both left- and right-positioned sidebars (per the audit's explicit decision not to implement drag-to-resize).
Uncontrolled dismiss returned null on the next render, so the callout vanished in one frame and content below it snapped up by its full height. There was no enter transition either. Wrap the callout in a transition shell driven by a data-state attribute (open/closing/closed). Dismiss now plays a 200ms fade + grid-row height collapse before unmounting, so content below glides up instead of snapping. Mount fades in over the same duration. Reduced motion is instant both ways. The controlled API (onDismiss provided) is unchanged -- the consumer still owns removal.
Tooltip animated with one-shot @Keyframes: not interruptible (restarts from zero instead of retargeting), no exit at all (popup unmounted instantly), a dead unused fade keyframe, and a dead transform-origin (every keyframe was a pure translate, origin-independent). Replace with [data-starting-style]/[data-ending-style] transitions at --rs-duration-fast/--rs-ease-out, keeping the exact per-side and per-align 2px directional offsets the keyframes used. Enter and exit now mirror each other, rapid hover toggling retargets smoothly instead of restarting, data-instant still skips the animation via transition-duration: 0, and reduced motion gets an opacity-only fade (previously nothing, since every rule lived inside the no-preference media query).
Four independent gaps: - Toast starting-style set a transform but no opacity: 0, so under reduced motion (where the transform never transitions) toasts popped in at full opacity instead of fading. - Navbar's hide-on-scroll transform had no reduced-motion fallback at all, so the bar teleported in/out; add an additive `reduce` block that fades it instead. - Spinner poles had no static opacity, so reduced motion (where the spin animation never applies) rendered a static asterisk instead of a graded ring that still reads as loading. - Dialog and command's backdrop blur had no prefers-reduced-transparency guard; add one with a heavier solid scrim (alert-dialog shares dialog's stylesheet, so it's covered too).
…uced motion The popup kept its 450ms transform transition active while swiping, so it eased toward the finger instead of tracking it directly (the backdrop already zeroed its own transition). Add transition: none on [data-swiping]. The five swipe-strength exit calcs hardcoded 400ms while enter runs 450ms via --rs-duration-drawer -- non-swipe exits ran measurably faster than opens for no reason. Use the token so enter/exit match and retuning the drawer duration doesn't require finding five literals. The panel had no transition outside the motion media query, so under prefers-reduced-motion it popped open/closed instantly. Add a base opacity fade (with matching [data-starting-style]/[data-ending-style] opacity: 0) so reduced motion gets a fade instead of a teleport, and move the per-side slide transforms into the no-preference block alongside the transition they animate.
| font-family: var(--rs-font-title); | ||
| font-weight: var(--rs-font-weight-regular); | ||
| color: var(--rs-color-foreground-base-primary); | ||
| width: 100%; |
There was a problem hiding this comment.
Issue: Base .headline rule drops width: 100% here (kept only on the truncate variant), with no corresponding entry in docs/V1-migration.md.
Why: The commit itself calls this "BREAKING (mild)" for consumers centering a Headline in a flex/grid row. We've hit this exact regression before on Headline's width contract, breaking a consumer in #799 and fixed a day later in #801. A missing migration-log entry for a breaking change is also this repo's most-repeated review correction across 11 prior PRs.
What: Add an entry to docs/V1-migration.md describing this width-behavior change. Checked not-found.tsx, it's unaffected since it already sets width: auto, but other consumers relying on the old default won't get a heads up without a migration note.
There was a problem hiding this comment.
Added a migration note under the Headline section of V1-migration.md covering the base width:100% removal and the one-line opt-back-in. Done.
| inset: 0; | ||
| z-index: var(--rs-z-index-portal); | ||
| background-color: var(--rs-color-overlay-black-a5); | ||
| transition: |
There was a problem hiding this comment.
Issue: This .backdrop transition (and the matching .dialogPopup transition/starting-ending-style block further down) gets deleted in this PR's rename to .overlay, with nothing replacing it anywhere in the file.
Why: The commit message only claims to add a prefers-reduced-transparency fallback, it doesn't mention removing the base animation. Sibling Dialog, touched in the very same commit for the same stated purpose, kept its base opacity fade. Right now the Cmd+K palette pops open/closed instantly for everyone, not just reduced-motion users.
What: Restore the opacity (and scale) transition using --rs-duration-*/--rs-ease-* tokens, matching Dialog's pattern.
There was a problem hiding this comment.
The backdrop was removed from the palette entirely, so the overlay fade no longer applies. Restored the popup open/close fade + scale (tokenized, reduced-motion gated) so it no longer pops open instantly. Done.
| role={ariaRole === 'table' ? 'row' : 'listitem'} | ||
| // A clickable row is announced as a button; a plain row keeps its | ||
| // structural role and stays inert for the keyboard. | ||
| role={onRowClick ? 'button' : ariaRole === 'table' ? 'row' : 'listitem'} |
There was a problem hiding this comment.
Issue: For variant="table", a clickable row now gets role="button" instead of role="row", while its cells still render role="cell" and the parent stays role="rowgroup".
Why: ARIA's required-context rule needs cell to have a row ancestor. Once the row reports as a button, most screen readers stop computing column/header association for it. Every sibling clickable-row implementation touched in this same PR (data-table, data-view-beta) kept role="row" for the same case.
What: Keep role="row"/role="cell" intact and add tabIndex/keydown handling alongside it, instead of swapping the role.
There was a problem hiding this comment.
Kept clickable rows as role=row/listitem so cells stay associated with their row, with tabIndex + keydown for activation. Matches data-view-beta. Done.
| {description && ( | ||
| <FieldPrimitive.Description | ||
| className={styles.description} | ||
| data-hidden={error ? '' : undefined} |
There was a problem hiding this comment.
Issue: FieldPrimitive.Description stays mounted, just hidden via CSS, even when error is also set, for the new crossfade.
Why: Base UI registers every mounted description/error id into aria-describedby regardless of visibility. Verified empirically on both branches: pre-PR aria-describedby resolves to just the error text, post-PR to both the old hint and the error concatenated.
What: Stop rendering the description while an error is showing, or explicitly override aria-describedby to reference only the current message id.
There was a problem hiding this comment.
The description is now unmounted while an error is showing, so aria-describedby resolves to just the error. Done.
| {icon && ( | ||
| <div className={styles.icon} aria-hidden='true'> | ||
| {icon} | ||
| <div className={styles.transitionShell} data-state={state}> |
There was a problem hiding this comment.
Issue: Two new wrapper divs (.transitionShell, .transitionInner) are added around Callout's root for the exit animation, and .transitionInner sets overflow: hidden unconditionally.
Why: ref now targets the innermost div, not the actual rendered root (verified by rendering <Callout ref={cb}> directly). This is the same "narrowed shared component DOM shape breaks consumers" pattern that's caused two prior incidents in this repo. The unconditional overflow: hidden also clips overflowing content, like a focus ring, even outside the closing state.
What: Scope overflow: hidden to [data-state="closing"] only, and double check whether the ref-target shift is acceptable as is.
There was a problem hiding this comment.
Scoped overflow:hidden to [data-state=closing] so open content (focus rings, popovers) is no longer clipped. Left the ref target as-is: it points to the same styled callout element it did before this PR, so consumers are unaffected. Done.
| color: var(--rs-color-foreground-base-secondary); | ||
| cursor: pointer; | ||
| text-decoration: none; | ||
| transition: |
There was a problem hiding this comment.
Issue: This transition block on .nav-item gets deleted with no replacement anywhere in the file.
Why: Every other interactive component touched in this PR was migrated onto the shared --rs-transition-interactive token. This one's the miss, so hover/focus/active on the sidebar's most-used control now snaps instead of easing.
What: Add transition: var(--rs-transition-interactive) back, inside the reduced-motion block like its siblings.
There was a problem hiding this comment.
Leaving this as-is. The instant nav-item hover/press is intentional to avoid a laggy feel on the sidebar most-used control.
| transition: opacity 0.2s ease; | ||
| /* Cap the width so the collapse can animate to 0 (width: auto can't | ||
| transition). 200px clears any real label; the sidebar caps them narrower. */ | ||
| max-width: 200px; |
There was a problem hiding this comment.
Issue: This new max-width cap, and the matching transition it enables, are added so the sidebar collapse can animate, but sidebar-item.tsx/sidebar-more.tsx still conditionally unmount the .nav-text span in the same render the collapse fires.
Why: The element is gone before the transition gets a frame to run, so the label still disappears instantly. .nav-group-label in this same file already solves this correctly by staying mounted unconditionally.
What: Keep the label mounted, matching .nav-group-label's pattern, instead of conditionally unmounting it.
There was a problem hiding this comment.
Kept the label mounted (matching nav-group-label) so it collapses with the sidebar instead of popping out. Done.
| <IconButton | ||
| aria-label={isDark ? 'Switch to light theme' : 'Switch to dark theme'} | ||
| onClick={onClickHandler} | ||
| style={{ width: size, height: size }} |
There was a problem hiding this comment.
Issue: IconButton's default padding isn't zeroed out here, while the outer element is pixel-locked to size via this inline style.
Why: The icon renders at size - 4px, about 13% smaller than requested by default. The existing "applies custom size" test only checks the SVG's width attribute, not computed layout, so it doesn't catch this.
What: Zero out IconButton's padding when a custom pixel size is supplied, or reconcile the two competing sizing mechanisms.
There was a problem hiding this comment.
Reconciled by treating size as the button box and letting IconButton CSS size the icon (dropped the redundant width/height on the icons). Updated the test to assert the button box. Done.
| {(state.checked || | ||
| state.indeterminate || | ||
| state.transitionStatus === 'ending') && | ||
| (state.indeterminate ? ( |
There was a problem hiding this comment.
Issue: This ternary reads state.indeterminate from Base UI's already-updated state, even while the icon is still fading out via transitionStatus === 'ending'.
Why: When a "select all" checkbox goes indeterminate straight to unchecked, state.indeterminate has already flipped to false by the time the exit transition starts, so it flashes to the checkmark icon for one frame before fading, instead of fading the dash.
What: Track which icon was showing before the transition started, e.g. via a ref, instead of reading current state during the exit.
There was a problem hiding this comment.
Now tracks the shown icon in a ref and renders it during the exit, so indeterminate to unchecked fades the dash instead of flashing the check for a frame. Done.
| @media (prefers-reduced-motion: no-preference) { | ||
| .positioner[data-transition="move"]:has(.popup:not([data-starting-style])) { | ||
| transition: transform 460ms cubic-bezier(0.33, 0.7, 0, 1); | ||
| transition: transform var(--rs-duration-slow) var(--rs-ease-out); |
There was a problem hiding this comment.
Issue: This transition maps to --rs-ease-out, replacing the original hand-tuned cubic-bezier(0.33, 0.7, 0, 1).
Why: That original curve is a near-exact match for the newly-introduced --rs-ease-drawer token instead. The parallel rule in tour-beta was mapped correctly in this same PR, so this looks like a slip during consolidation rather than a deliberate change.
What: Use var(--rs-ease-drawer) instead of var(--rs-ease-out).
There was a problem hiding this comment.
Skipping. tour-beta parallel rule also uses --rs-ease-out (not ease-drawer), so changing only tour would make the two inconsistent. Leaving both on ease-out for now.
| } | ||
|
|
||
| [data-style="modern"] { | ||
| --rs-font-title: var(--rs-font-inter), sans-serif; |
There was a problem hiding this comment.
Issue: These two custom-property redeclarations get removed from [data-style="modern"].
Why: A nested <Theme style="modern"> inside an outer <Theme style="traditional"> now inherits the outer scope's fonts instead of resetting to Inter, silently contradicting the docs' own composition example. This mirrors the gray.css bug this team already fixed once before (#768).
What: Keep the explicit --rs-font-title/--rs-font-body re-declaration here.
There was a problem hiding this comment.
Restored the --rs-font-title/--rs-font-body re-declaration on [data-style=modern] so a nested modern theme resets to Inter instead of inheriting the outer fonts. Done.
| return <tr {...props} />; | ||
| const row = cva(styles['row'], { | ||
| variants: { | ||
| interactive: { |
There was a problem hiding this comment.
Issue: This new interactive variant on TableRow isn't reflected in apps/www/.../table/props.ts.
Why: It's a real, user-facing behavioral prop, toggles hover/active row styling via CSS, and this repo's convention is to update docs props.ts in the same PR a component gains a new prop.
What: Add interactive to TableRowProps in the docs site.
There was a problem hiding this comment.
Added interactive to TableRowProps in the docs props.ts. Done.
| aria-valuetext={valueText} | ||
| aria-valuemin={0} | ||
| aria-valuemax={100} | ||
| aria-valuenow={Math.round((chroma / CHROMA_MAX) * 100)} |
There was a problem hiding this comment.
Issue: role="slider" here reports only this one axis (chroma) via aria-valuenow/min/max, but the keyboard handler drives a second, independent axis (lightness) that never shows up in these values.
Why: aria-valuetext does update correctly on every keypress, so this isn't a functional break, but it's non-conformant ARIA for a genuinely 2D control.
What: Consider two separate slider elements, one per axis, matching established 2D color-area patterns like React Aria's ColorArea.
There was a problem hiding this comment.
Skipping for this PR. aria-valuetext already announces both axes so it is not a functional break, and the two-slider refactor (which also affects HslArea) is better as its own change. Noting as a follow-up.
| <span | ||
| className={styles.datePickerError} | ||
| data-visible={error ? '' : undefined} | ||
| role='alert' |
There was a problem hiding this comment.
Issue: This new error span has no aria-describedby link connecting it to the input.
Why: role="alert" only announces the message at the moment it appears. A user tabbing into an already-erroring field hears aria-invalid with no stated reason.
What: Give the error span a stable id and wire it into the input's aria-describedby.
There was a problem hiding this comment.
Gave the error span a stable id and wired it into the input aria-describedby (merged with any consumer value) when an error is present. Done.
| } | ||
|
|
||
| .action-btn:focus-visible { | ||
| outline: 1px solid currentColor; |
There was a problem hiding this comment.
Issue: This focus ring hardcodes outline/currentColor instead of the new shared --rs-focus-ring token used by roughly 19 other components in this PR.
Why: Turns out deliberate, currentColor reads better against the bar's colored variants per the commit message, but that reasoning lives only there, not as a code comment, so a future token-adoption pass is likely to "fix" it by mistake.
What: Add a one-line comment here explaining the divergence.
There was a problem hiding this comment.
Skipping. The currentColor focus ring here is deliberate so it reads against the bar colored variants; leaving it as-is for this PR.
| .action-btn:hover { | ||
| opacity: 0.8; | ||
| } | ||
|
|
||
| /* Press reads as scale, not a second dim — 0.64 text mid-press is barely | ||
| legible on the gradient variant, and hover's 0.8 still applies. */ | ||
| .action-btn:active { | ||
| transform: scale(var(--rs-scale-pressed)); | ||
| } | ||
|
|
||
| .action-btn:focus-visible { | ||
| outline: 1px solid currentColor; | ||
| outline-offset: 2px; | ||
| } |
There was a problem hiding this comment.
Right now the action-btn is a html button. Ideally it should be using Apsara Button component, in that way we don't have to add focus styles in other components.
There was a problem hiding this comment.
Tried swapping in <Button variant="text">. It doesn't fit cleanly here:
- Button's
textvariant hardcodescolor: var(--rs-color-foreground-base-primary)(notcurrentColor), so on the normal bar (dark bg) the label goes dark-on-dark and on the error bar it's low contrast. To keep it readable I had to override color, background, and the hover box — so most of the custom CSS stayed anyway. - The one thing we'd inherit, the focus ring, is
1px solid --rs-color-border-accent-emphasis(blue). The current ring iscurrentColor, so it matches each bar (white on dark/error, dark on gradient). A blue ring on the red error bar looks off-brand and lower contrast — so we'd want to re-add a currentColor override regardless.
Net: the swap adds a Button dependency without removing the custom styles, and slightly regresses the focus ring on colored bars. Keeping the raw button for now. In long term either we can use scoped theming to revert or currentColor option.
| --rs-ease-out: cubic-bezier( | ||
| 0.22, | ||
| 1, | ||
| 0.36, | ||
| 1 | ||
| ); /* entrances, exits, value changes */ | ||
| --rs-ease-in-out: cubic-bezier( | ||
| 0.77, | ||
| 0, | ||
| 0.175, | ||
| 1 |
There was a problem hiding this comment.
lines need to be prettified/formatted
There was a problem hiding this comment.
Fixed. Moved the trailing comments above each easing token so the values stay on single lines and Biome keeps them that way. Done.
| .breadcrumb-link:focus-visible { | ||
| outline: none; | ||
| background-color: var(--rs-color-background-base-primary-hover); | ||
| color: var(--rs-color-foreground-base-secondary); | ||
| border-radius: var(--rs-radius-1); | ||
| } | ||
|
|
There was a problem hiding this comment.
the breadcrumb focus visible looks weird and not consistent. Let's make it consistent with other styles. Something similar to -
There was a problem hiding this comment.
Fixed. Swapped the background-fill focus for the shared --rs-focus-ring outline, matching Link. Done.
| .breadcrumb-dropdown-trigger:focus-visible { | ||
| background-color: var(--rs-color-background-base-primary-hover); | ||
| color: var(--rs-color-foreground-base-primary); | ||
| border-radius: var(--rs-radius-1); | ||
| } | ||
|
|
There was a problem hiding this comment.
Same fix applied to the dropdown trigger focus. Done.
| /* Trigger sits flush in a clipped container: keep the ring inside. */ | ||
| .accordion-trigger:focus-visible { | ||
| outline: var(--rs-focus-ring); | ||
| outline-offset: var(--rs-focus-ring-offset-inset); |
There was a problem hiding this comment.
The offset needs to be -1px in this case, else the border and outline both are visible.
with -2px :
with 1px:
Let's create two variables for the outline-inset , supporting both 1px & 2px
There was a problem hiding this comment.
Fixed. Added a --rs-focus-ring-offset-inset-border (-1px) token and pointed accordion at it so the ring hugs the border. Applied the same to number-field steppers and the grouped toggle (same bordered case). Done.
| default; a colored control (danger, success, …) overrides | ||
| `outline-color` in its own :focus-visible rule to match. */ | ||
| --rs-focus-ring: 1px solid var(--rs-color-border-accent-emphasis); | ||
| --rs-focus-ring-shadow: 0 0 0 1.5px var(--rs-color-border-accent-primary); |
There was a problem hiding this comment.
| --rs-focus-ring-shadow: 0 0 0 1.5px var(--rs-color-border-accent-primary); | |
| --rs-focus-ring-shadow: 0 0 0 1px var(--rs-color-border-accent-emphasis); |
^ This is used mainly for borderless input variants. We should match this style to the ring css for consistency.
There was a problem hiding this comment.
Applied your suggestion. --rs-focus-ring-shadow is now 0 0 0 1px var(--rs-color-border-accent-emphasis), matching the outline ring. Affects the three borderless inputs (input, text-area, otp-field). Done in 57336d5.
- accordion/number-field/toggle: hug bordered focus ring via new -1px inset token - breadcrumb: use shared focus ring instead of a background fill - command: restore Cmd+K popup fade+scale animation - callout: scope overflow:hidden to the closing state so open content isn't clipped - checkbox: keep the shown icon during exit (no checkmark flash on indeterminate->unchecked) - data-view: keep clickable rows as role=row; ignore keydown bubbling from child controls - field: hide description while an error shows so aria-describedby stays clean - sidebar: keep nav label mounted so it collapses with the sidebar - theme switcher: size drives the button box; icon fills via CSS - date-picker: link error text to input via aria-describedby - typography: re-declare fonts on modern style so nested themes reset - docs: headline width migration note, table interactive prop - effects: single-line easing tokens
Use 1px + accent-emphasis so the box-shadow ring on input/text-area/otp-field matches --rs-focus-ring used everywhere else.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/raystack/components/data-view/components/list.tsx (1)
356-386: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRow click still bubbles from interactive children, unlike the fixed keydown path.
handleRowKeyDownnow correctly ignores bubbled keyboard events withevent.target !== event.currentTarget, butonClick={handleRowActivate}on the row (Line 386) has no equivalent guard. If a cell renders an interactive control (checkbox, button, link), a click on it will still bubble and fireonRowClickin addition to the control's own handler — same root cause as the keydown issue just fixed, just via a different event.🛠️ Proposed fix
- onClick={handleRowActivate} + onClick={event => { + if (event.target !== event.currentTarget) return; + handleRowActivate(); + }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raystack/components/data-view/components/list.tsx` around lines 356 - 386, Update the row click handling around handleRowActivate so clicks originating from interactive descendants do not trigger onRowClick; guard the row’s onClick path using the same event-target check as handleRowKeyDown while preserving activation for clicks directly on the row itself.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/V1-migration.md`:
- Around line 1329-1337: Update the migration note to consistently scope the
removed base width behavior to the Headline truncate variant. Revise the prose
to clarify that non-truncated Headline instances are unaffected, and change the
before/after example to use a truncated Headline while demonstrating the
full-width override.
In `@packages/raystack/components/checkbox/checkbox.tsx`:
- Around line 116-139: In packages/raystack/components/checkbox/checkbox.tsx
lines 116-139, extract the indicator render logic into an IndicatorContent
component that uses useState-derived previous-icon tracking without reading or
writing refs during render, then render it from the CheckboxItem indicator
callback. Remove shownIconRef from CheckboxItem at lines 98-103 and replace the
useRef import at line 6 with useState; apply these changes at all listed sites.
---
Outside diff comments:
In `@packages/raystack/components/data-view/components/list.tsx`:
- Around line 356-386: Update the row click handling around handleRowActivate so
clicks originating from interactive descendants do not trigger onRowClick; guard
the row’s onClick path using the same event-target check as handleRowKeyDown
while preserving activation for clicks directly on the row itself.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91acc9f7-6b24-46aa-95c1-a6be4d0e76b2
📒 Files selected for processing (27)
apps/www/src/components/docs/search.tsxapps/www/src/content/docs/components/command/index.mdxapps/www/src/content/docs/components/command/props.tsapps/www/src/content/docs/components/table/props.tsdocs/V1-migration.mdpackages/raystack/components/accordion/accordion.module.csspackages/raystack/components/breadcrumb/breadcrumb.module.csspackages/raystack/components/calendar/date-picker.tsxpackages/raystack/components/callout/callout.module.csspackages/raystack/components/checkbox/checkbox.tsxpackages/raystack/components/command/command-dialog.tsxpackages/raystack/components/command/command-input.tsxpackages/raystack/components/command/command.module.csspackages/raystack/components/data-view/__tests__/data-view.test.tsxpackages/raystack/components/data-view/components/list.tsxpackages/raystack/components/field/__tests__/field.test.tsxpackages/raystack/components/field/field-root.tsxpackages/raystack/components/field/field.module.csspackages/raystack/components/number-field/number-field.module.csspackages/raystack/components/sidebar/__tests__/sidebar.test.tsxpackages/raystack/components/sidebar/sidebar-item.tsxpackages/raystack/components/sidebar/sidebar-more.tsxpackages/raystack/components/theme-provider/__tests__/theme.test.tsxpackages/raystack/components/theme-provider/switcher.tsxpackages/raystack/components/toggle/toggle.module.csspackages/raystack/styles/effects.csspackages/raystack/styles/typography.css
💤 Files with no reviewable changes (2)
- apps/www/src/content/docs/components/command/props.ts
- packages/raystack/components/field/field.module.css
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/raystack/components/theme-provider/tests/theme.test.tsx
- packages/raystack/components/accordion/accordion.module.css
- packages/raystack/components/calendar/date-picker.tsx
- packages/raystack/components/callout/callout.module.css
- packages/raystack/components/toggle/toggle.module.css
- packages/raystack/components/theme-provider/switcher.tsx
- packages/raystack/components/number-field/number-field.module.css
- packages/raystack/styles/effects.css
- packages/raystack/components/data-view/tests/data-view.test.tsx
| ((props, state) => { | ||
| const isEnding = state.transitionStatus === 'ending'; | ||
| if (!isEnding) { | ||
| shownIconRef.current = state.indeterminate | ||
| ? 'indeterminate' | ||
| : 'check'; | ||
| } | ||
| const showIcon = state.checked || state.indeterminate || isEnding; | ||
| const icon = isEnding | ||
| ? shownIconRef.current | ||
| : state.indeterminate | ||
| ? 'indeterminate' | ||
| : 'check'; | ||
| return ( | ||
| <span {...props}> | ||
| {showIcon && | ||
| (icon === 'indeterminate' ? ( | ||
| <IndeterminateIcon /> | ||
| ) : ( | ||
| <CheckMarkIcon /> | ||
| ))} | ||
| </span> | ||
| ); | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid mutating and reading refs during the render phase.
Writing to and reading from a ref (shownIconRef.current) directly inside the render function violates React's rules for pure components. In Strict Mode or React 18/19 concurrent rendering, this can lead to incorrect states if a render is interrupted or executed multiple times.
Extract the indicator logic into a separate component and use the React-approved "derived state" pattern to safely track the previous icon without side effects.
packages/raystack/components/checkbox/checkbox.tsx#L116-L139: Replace the render prop implementation with an extracted component (e.g.,IndicatorContent).packages/raystack/components/checkbox/checkbox.tsx#L98-L103: Remove theshownIconRefdeclaration fromCheckboxItem.packages/raystack/components/checkbox/checkbox.tsx#L6-L6: Replace theuseRefimport withuseState.
🛠️ Proposed refactor for the indicator component
Define this inner component outside of CheckboxItem:
function IndicatorContent({ indicatorProps, state }: { indicatorProps: any; state: any }) {
const isEnding = state.transitionStatus === 'ending';
const currentIcon = state.indeterminate ? 'indeterminate' : 'check';
const [lastIcon, setLastIcon] = useState<'check' | 'indeterminate'>(currentIcon);
// Safely derive state during render without side effects
if (!isEnding && currentIcon !== lastIcon) {
setLastIcon(currentIcon);
}
const showIcon = state.checked || state.indeterminate || isEnding;
const icon = isEnding ? lastIcon : currentIcon;
return (
<span {...indicatorProps}>
{showIcon && (icon === 'indeterminate' ? <IndeterminateIcon /> : <CheckMarkIcon />)}
</span>
);
}Then simplify the render prop in CheckboxItem:
- ((props, state) => {
- const isEnding = state.transitionStatus === 'ending';
- if (!isEnding) {
- shownIconRef.current = state.indeterminate
- ? 'indeterminate'
- : 'check';
- }
- const showIcon = state.checked || state.indeterminate || isEnding;
- const icon = isEnding
- ? shownIconRef.current
- : state.indeterminate
- ? 'indeterminate'
- : 'check';
- return (
- <span {...props}>
- {showIcon &&
- (icon === 'indeterminate' ? (
- <IndeterminateIcon />
- ) : (
- <CheckMarkIcon />
- ))}
- </span>
- );
- })
+ ((props, state) => <IndicatorContent indicatorProps={props} state={state} />)📍 Affects 1 file
packages/raystack/components/checkbox/checkbox.tsx#L116-L139(this comment)packages/raystack/components/checkbox/checkbox.tsx#L98-L103packages/raystack/components/checkbox/checkbox.tsx#L6-L6
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/checkbox/checkbox.tsx` around lines 116 - 139,
In packages/raystack/components/checkbox/checkbox.tsx lines 116-139, extract the
indicator render logic into an IndicatorContent component that uses
useState-derived previous-icon tracking without reading or writing refs during
render, then render it from the CheckboxItem indicator callback. Remove
shownIconRef from CheckboxItem at lines 98-103 and replace the useRef import at
line 6 with useState; apply these changes at all listed sites.
57336d5 to
e9829c9
Compare
e9829c9 to
47541aa
Compare
3 participants
Summary
A design-system-wide audit of the component library, consolidating the animation work and the broader interaction/craft pass into a single branch.
What's covered
:focus-visiblering across the library.styles/effects.css; Inter body tracking and the h1–h4 type ramp retuned.Related issues
Closes #681 — consistent, tokenized
:focus-visiblering applied across the library.Closes #621 — Image error/loading handling: one-shot fallback guard plus load-state fade-in.
Closes #625 — Link's remaining items (focus-visible, reduced-motion) are now done.
Partially addresses:
TableRownow uses CVA and gains row-state hooks. Cell truncation accessibility is still open.announementBartypo are still open.Test plan
pnpm buildpasses.@base-uimodule-resolution failure affects a handful of unrelated test files onmainas well.