[PF-1994] Migrate Slider to @base-ui/react + Tailwind#4955
Conversation
🦋 Changeset detectedLatest commit: fa60f0e The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
|
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
| </span> | ||
| <input | ||
| aria-orientation="horizontal" | ||
| aria-valuenow="0" |
There was a problem hiding this comment.
aria-valuemax="100"
aria-valuemin="0"
aria-valuenow="0"
I think those were useful
There was a problem hiding this comment.
🤖 Orchestrator agent (autonomous review-response)
Good eye. This is a base-ui behavior change — its <Slider.Thumb> renders an <input type="range"> with native max/min/aria-valuenow but no longer emits explicit aria-valuemin/aria-valuemax. Per the HTML AAM (and WAI-ARIA 1.2), implicit aria-valuemax/aria-valuemin are derived from the native max/min attributes on <input type="range">, so screen readers see the equivalent semantics through the native attributes that are still there. Base-ui doesn't expose props to pass arbitrary attrs to its inner input.
Two options:
- Leave as-is — rely on native
max/min(spec-compliant; what base-ui ships). - Patch back — reach into base-ui's input via the
inputRefprop andsetAttributeimperatively in an effect (works, but somewhat hacky and fragile against future base-ui releases).
Leaning toward leave-as-is given native attrs are the canonical source. Want me to add the imperative patch anyway? 👍 to confirm, or share thoughts.
There was a problem hiding this comment.
🤖 Orchestrator agent (autonomous review-response)
Update: the orchestrator's Happo gate flagged the migrated DOM as a regression (pixel-perfect is required for a UI-kit refactor). Inspecting the diff pairs revealed two real issues that fixing the visuals — also restoring the centered thumb that you observed — addresses:
- Thumb position offset. Base-ui's
<Slider.Thumb>ships withstyle="inset-inline-start: X%; top: 50%", which puts the thumb's top-left at the anchor — not centered. Added-translate-x-1/2 -translate-y-1/2to recenter on the anchor point (parity with the prior-mt -mlbehavior in @mui/base). - Indicator opacity inheritance. Base-ui nests
<Slider.Indicator>inside<Slider.Track>. Ouropacity-[0.24]on the track was faded the blue indicator child to 24% (most visible on the Range story — the connecting blue line between thumbs was nearly invisible). Moved the opacity onto the color (bg-gray-500/[0.24]) so it only affects the gray fill, not descendants.
Re: the aria-valuemin/max — they remain implicit via the native max/min on <input type="range"> (no base-ui prop to forward arbitrary aria attrs). This is unchanged by the visual fix and still equivalent semantics per HTML-AAM. Leaving as-is. Pushing the CSS fix in the next gate run.
There was a problem hiding this comment.
🤖 Orchestrator agent (autonomous review-response)
Correction to last iteration: the Tailwind -translate-x-1/2 -translate-y-1/2 I added to the thumb was wrong and got reverted. Root cause: jsdom doesn't serialize CSS Transforms 2's individual-axis translate: property into the inline-style string, so my Jest snapshot read showed no centering — but base-ui's <Slider.Thumb> source (node_modules/@base-ui/react/slider/thumb/SliderThumb.js) actually sets translate: ${rtl ? 1 : -1}*50% ${vertical ? 1 : -1}*50% directly, which Chrome applies. My added Tailwind translates composed with it (CSS translate: and transform: translate() are independent) and over-shifted the thumb. Also bit me: Picasso's jss-snapshot-serializer.cjs strips Tailwind suffixes (-translate-x-1/2 → -translate-x), which made my snapshot update wrong and Static checks fail — confirmed by job 76089179101.
What stays from last iteration: the indicator opacity fix. <Slider.Indicator> is now a CHILD of <Slider.Track> (it was a sibling in @mui/base), so the Track's opacity-[0.24] was being inherited by the blue indicator (faded to 24%, visible as the missing blue connector between thumbs on the Range story). Kept the fix opacity-[0.24] bg-gray-500 → bg-gray-500/[0.24] so opacity stays on the bg color only.
Net diff vs. master: one Tailwind class change on the Track (opacity-[0.24] bg-gray-500 → bg-gray-500/[0.24]). Aria-valuemin/max stays implicit via native max/min — unchanged from prior thread.
There was a problem hiding this comment.
I'd like to keep having
aria-valuemax="100"
aria-valuemin="0"
aria-valuenow="0"
for accessibility purposes
There was a problem hiding this comment.
🤖 Orchestrator agent (autonomous review-response)
Done — added explicit aria-valuemin, aria-valuemax, aria-valuenow to each Slider.Thumb. Snapshot updated to show all three attributes on the thumb div.
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
Happo fix — computed-style diagnosticFollowing the new pixel-perfect protocol, ran a Single concrete diff identified on the thumb element:
OLD NEW Net: NEW thumb is 1.5px left of baseline → drives all 8 Storybook diffs (Default, Initial value, Marks, Range, Tooltip, Controlled, Disable track, Hide thumb) plus the Cypress Fix ( const thumbClassName = twJoin(
'group/thumb flex justify-center items-center w-[15px] h-[15px]',
'rounded-[50%] bg-blue-500 border-[2px] border-solid border-white',
- 'outline-0 absolute transition-shadow cursor-pointer',
+ 'outline-0 absolute transition-shadow cursor-pointer ml-[1.5px]',
isThumbHidden && 'hidden'
)Math: Verified live via Playwright after rebuild:
Snapshot updated to include |
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
Happo fix iter-7 — full computed-style diagnosticNow that the Happo upload completed for the previous head, fresh diff PNGs are available. Ran 1. Track color-space mismatch (Tailwind v4 OKLAB)
Reverting Track to 2. Thumb horizontal offset (asymmetric centering)
Fix: 3. Mark vertical offset (Control vs root parent)
Fix: 4. Bonus — root height +1pxRemoving flow contribution from Control with Verification (Playwright, post-rebuild)Files changed:
Diagnostic data saved to |
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
Happo fix iter-8 — input rect mismatchPrior iteration closed 2 of 9 diffs (Cypress range/tooltip + "Hide thumb" Storybook). Remaining 7 stories all have a visible thumb. Re-ran One more diff identified:
OLD Both clip the input to invisible (legacy Fix in const thumbClassName = twJoin(
'group/thumb flex justify-center items-center w-[15px] h-[15px]',
'rounded-[50%] bg-blue-500 border-[2px] border-solid border-white',
'outline-0 absolute transition-shadow cursor-pointer ml-[1.5px]',
+ '[&_input]:!top-auto [&_input]:!left-auto',
isThumbHidden && 'hidden'
)
Playwright verification (post-rebuild):
Snapshot updated to include the new utility (jss-snapshot-serializer leaves Net accumulated fix list across all iterations:
|
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
Tier 0 component. See PR description for prop-surface diff, import diff, and Happo summary. Refs: PF-1994
Tier 0 component. See PR description for prop-surface diff, import diff, and Happo summary. Refs: PF-1994 [review-iter 1] address review feedback
Tier 0 component. See PR description for prop-surface diff, import diff, and Happo summary. Refs: PF-1994 [review-iter 2] address review feedback
Tier 0 component. See PR description for prop-surface diff, import diff, and Happo summary. Refs: PF-1994 [review-iter 3] address review feedback
Tier 0 component. See PR description for prop-surface diff, import diff, and Happo summary. Refs: PF-1994 [review-iter 4] address review feedback
|
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
| // eslint-disable-next-line react/no-array-index-key | ||
| key={`thumb-${index}`} | ||
| index={index} | ||
| id={thumbId} |
There was a problem hiding this comment.
The id prop is documented as "Id attribute of the input element," but it's now passed to <Slider.Thumb>, which puts it on the thumb div rather than the input. It also only goes to the first thumb (index === 0 ? id : undefined). Was this placement intentional? If anything targets #id expecting the input (or the old root element), this changes where it lands.
There was a problem hiding this comment.
🤖 Orchestrator agent (autonomous review-response)
Good catch. In @base-ui/react, id on Slider.Thumb goes to the thumb div, not the inner <input type="range"> (which gets an auto-generated id). To forward id to the input we'd need an imperative inputRef callback:
<Slider.Thumb inputRef={(el) => { if (el && thumbId) el.id = thumbId }} ...>This preserves the documented semantics. Apply this? 👍 to confirm, or let me know if you'd prefer to update the prop docs to reflect the new placement instead.
There was a problem hiding this comment.
nope, this is not great. let's keep this way OR maybe we can put the id on the root? that would make more sense, don't you think?
Tier 0 component. See PR description for prop-surface diff, import diff, and Happo summary. Refs: PF-1994 [review-iter 12] address review feedback
Two issues addressed in latest push: Happo Danger commit-message failure: commit |
Tier 0 component. See PR description for prop-surface diff, import diff, and Happo summary. Refs: PF-1994 [review-iter 12] address review feedback
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
Danger fails on commit |
| <MUIBaseSlider | ||
| ref={sliderRef} | ||
| <BaseUISlider.Root | ||
| ref={ref as React.Ref<HTMLDivElement>} |
There was a problem hiding this comment.
type casting is not allowed in Picasso codebase
denieler
left a comment
denieler
left a comment
There was a problem hiding this comment.
The clicking area for thumbs in Slider before this change was significantly bigger, so it was easier to use Slide as a user. I think this is necessary to fix in this PR
Done — root cause confirmed. |
Tier 0 component. See PR description for prop-surface diff, import diff, and Happo summary. Refs: PF-1994 [review-iter 14] address review feedback
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
🛑 Orchestrator escalation —
|
🛑 Orchestrator escalation —
|
🛑 Orchestrator escalation —
|
🛑 Orchestrator escalation —
|
🛑 Orchestrator escalation —
|
Danger fails on commit |
Playwright runtime verification on |
Re-verified at Happo's The persistent Operator cleanup needed: commit |
Tier 0 component. See PR description for prop-surface diff, import diff, and Happo summary. Refs: PF-1994 [review-iter 15] address review feedback
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
🛑 Orchestrator escalation —
|
🛑 Orchestrator escalation —
|
3 participants
Slider migration diff
Generated: 2026-05-13 14:41:19 CEST
Package:
packages/base/SliderFiles
No file additions, deletions, or renames.
Imports
Removed:
Added:
MUI v4 / JSS residue check
@material-ui/*source imports@material-ui/corein package.jsonMigration is NOT complete until all three are 0.
package.json delta
Prop-surface diff
Click to expand .d.ts diff
Review carefully: any
-line on a public export is a breaking change. Seedocs/migration/rules/api-preservation.md.Happo
Happo log:
migration-runs/2026-05-13/Slider/happo.log(0? flagged lines).
Designer: review screen diffs >0.5% per
docs/migration/migration-plan.md§6.3.React 19 smoke
Stubbed (pending PF-1994). The real smoke wires up during PF-1994's first migration.