Skip to content
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .changeset/slider-migration.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
'@toptal/picasso-slider': major
---

### Slider

- Migrate internals from @mui/base to @base-ui/react (behavioral parity)
- Rebuild marks and value-label rendering on top of @base-ui/react's compound parts (Slider.Root + Slider.Control + Slider.Track + Slider.Indicator + Slider.Thumb)
6 changes: 6 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,9 @@ migration-runs
# debris example.
.playwright-mcp

# Per-sweep temp files the review-response agent writes for `gh pr comment
# --body-file` (sandbox blocks shell-redirection to outside the worktree
# and blocks rm/mv, so the file is written here, consumed by gh, and left
# behind). Same rationale as .playwright-mcp.
*.tmp.md

4 changes: 2 additions & 2 deletions packages/base/Slider/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
},
"homepage": "https://github.com/toptal/picasso/tree/master/packages/picasso#readme",
"dependencies": {
"@mui/base": "5.0.0-beta.58",
"@base-ui/react": "^1.4.1",
"@toptal/picasso-shared": "15.0.0",
"@toptal/picasso-tooltip": "2.0.5",
"@toptal/picasso-utils": "4.0.0"
Expand All @@ -32,7 +32,7 @@
"@toptal/picasso-tailwind-merge": "^2.0.0",
"@toptal/picasso-provider": "*",
"@toptal/picasso-tailwind": ">2.1.0",
"react": ">=16.12.0 < 19.0.0"
"react": ">=16.12.0"
},
"exports": {
".": "./dist-package/src/index.js"
Expand Down
212 changes: 158 additions & 54 deletions packages/base/Slider/src/Slider/Slider.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// import type { ComponentProps } from 'react'
import React, { forwardRef, useRef } from 'react'
import { Slider as MUIBaseSlider } from '@mui/base/Slider'
import { useCombinedRefs, useOnScreen } from '@toptal/picasso-utils'
import { Slider as BaseUISlider } from '@base-ui/react/slider'
import type { SliderRootChangeEventDetails } from '@base-ui/react/slider'
import { useOnScreen } from '@toptal/picasso-utils'
import { twJoin, twMerge } from '@toptal/picasso-tailwind-merge'
import type { BaseProps } from '@toptal/picasso-shared'

Expand Down Expand Up @@ -56,6 +56,61 @@ export interface Props extends BaseProps {
id?: string
}

const formatValueLabel = (
value: number,
index: number,
format?: string | ((value: number, index: number) => React.ReactNode)
): React.ReactNode => {
if (typeof format === 'function') {
return format(value, index)
}

if (typeof format === 'string') {
return format
}

return value
}

const generateMarkPositions = (
min: number,
max: number,
step: number
): number[] => {
const positions: number[] = []
const range = max - min

if (range <= 0 || step <= 0) {
return positions
}

for (let mark = min; mark <= max; mark += step) {
positions.push(mark)
}

return positions
}

const resolveThumbValues = (
value: number | number[] | undefined,
defaultValue: number | number[],
min: number
): number[] => {
if (Array.isArray(value)) {
return value
}

if (typeof value === 'number') {
return [value]
}

if (Array.isArray(defaultValue)) {
return defaultValue
}

return [typeof defaultValue === 'number' ? defaultValue : min]
}

export const Slider = forwardRef<HTMLElement, Props>(function Slider(
{ defaultValue = 0, min = 0, max = 100, tooltip = 'off', ...props },
ref
Expand All @@ -64,7 +119,7 @@ export const Slider = forwardRef<HTMLElement, Props>(function Slider(
marks,
value,
tooltipFormat,
step,
step = 1,
disabled,
onChange,
onBlur,
Expand All @@ -79,7 +134,6 @@ export const Slider = forwardRef<HTMLElement, Props>(function Slider(
'data-testid': dataTestid,
} = props
const containerRef = useRef<HTMLDivElement>(null)
const sliderRef = useCombinedRefs<HTMLElement>(ref, useRef<HTMLElement>(null))

// The rootMargin is not working correctly in the storybooks iframe
// To test properly we can open the iframe in new window
Expand All @@ -98,69 +152,119 @@ export const Slider = forwardRef<HTMLElement, Props>(function Slider(
const isThumbHidden =
hideThumbOnEmpty && (typeof value === 'undefined' || value === null)

const isRange = Array.isArray(value) || Array.isArray(defaultValue)
const thumbValues = resolveThumbValues(value, defaultValue, min)

const handleValueChange = (
newValue: number | readonly number[],
eventDetails: SliderRootChangeEventDetails
) => {
if (!onChange) {
return
}

const normalizedValue: number | number[] = Array.isArray(newValue)
? [...newValue]
: (newValue as number)

onChange(eventDetails.event, normalizedValue, eventDetails.activeThumbIndex)
}

// Override base-ui's inline `translate: -50% -50%` + `top: 50%` (creates a
// transform stacking context that affects descendant tooltip box-shadow
// rasterization) and mirror baseline's margin-based positioning so the
// thumb renders without a transform — keeps tooltip shadow pixel-identical
// to pre-migration baseline.
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',
'![translate:none] ![top:-7px] -ml-[6px]',
'[&_input]:!top-auto [&_input]:!left-auto [&_input]:![clip-path:none] [&_input]:[clip:rect(0,0,0,0)]',
isThumbHidden && 'hidden'
)

const indicatorClassName = twJoin(
'block !absolute h-[1px]',
disableTrackHighlight ? 'bg-gray-200' : 'bg-blue-500'
)

const markPositions = marks ? generateMarkPositions(min, max, step) : []

return (
<div
ref={containerRef}
className={twMerge('my-[6px] mx-0', className)}
style={style}
>
<MUIBaseSlider
ref={sliderRef}
<BaseUISlider.Root
ref={ref as React.Ref<HTMLDivElement>}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

type casting is not allowed in Picasso codebase

defaultValue={defaultValue}
value={value}
min={min}
max={max}
step={step}
marks={marks}
disabled={disabled}
name={name}
data-testid={dataTestid}
data-private={dataPrivate}
onFocus={onFocus}
onBlur={onBlur}
name={name}
id={id}
slots={{
mark: SliderMark,
valueLabel: SliderValueLabel,
}}
slotProps={{
mark: {
// @ts-expect-error we have custom Mark component, where we extend props and MUI does not understand it
forceInactive: disableTrackHighlight,
},
root: {
className:
'block cursor-pointer width-full relative py-[6px] -my-[6px]',
},
rail: {
className:
'block absolute w-full h-[1px] opacity-[0.24] rounded-none bg-gray-500',
},
thumb: {
className: twJoin(
'group/thumb flex justify-center items-center w-[15px] h-[15px]',
'rounded-[50%] bg-blue-500 border-[2px] border-solid border-white',
'-mt-[7px] -ml-[6px] outline-0 absolute transition-shadow cursor-pointer',
isThumbHidden && 'hidden'
),
role: 'slider',
},
track: {
className: twJoin(
'block absolute h-[1px]',
disableTrackHighlight ? 'bg-gray-200' : 'bg-blue-500'
),
},
valueLabel: {
tooltip: isObserved ? tooltip : 'off',
onRender: handleValueLabelOnRender,
yPlacement: isOnScreen ? 'top' : 'bottom',
isOverlaped: isPartiallyOverlapped,
},
}}
valueLabelFormat={tooltipFormat}
onChange={onChange}
/>
onValueChange={handleValueChange}
className='block cursor-pointer w-full relative py-[6px] -my-[6px]'
>
<BaseUISlider.Control className='block w-full relative h-[1px] -mb-[1px]'>
<BaseUISlider.Track className='block !absolute w-full h-[1px] opacity-[0.24] rounded-none bg-gray-500' />
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think absolute positioning is wrong here, as it won't place the track correctly if the component will be used on the page in various layout locations. Also, we shouldn't use important decorations as it's a bad practice to force override the style

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

🤖 Orchestrator agent (autonomous review-response)

Both points are fair. Let me address them separately:

On !important — agreed, the Tailwind ! modifiers (!absolute on Track + Indicator, ![translate:none] ![top:-7px] on Thumb) are a code smell. They were added to override style that @base-ui/react sets inline via its own render. Turns out the bang isn't actually needed: base-ui's useRenderElement merges the user-passed style prop AFTER its internal defaults (mergeObjects(outProps.style, userStyle) in useRenderElement.js:86), so an inline style={{ position: 'absolute' }} on Track wins over base-ui's default position: 'relative' without !important. Same for Indicator and Thumb. Only the [&_input]:!* rules (overriding base-ui's spread of visuallyHidden onto the inner <input type="range">) would still need ! since we don't own that input — base-ui spreads inline styles onto it and we can't reach in without imperative inputRef.setAttribute(), which is fragile.

On position: absolute being layout-fragile — the absolute Track is anchored to <BaseUISlider.Control className="… relative …"> one level up (line 214). Control establishes the positioning context, so Track + Indicator + Thumbs overlap on the same 1px row regardless of where Slider sits in the parent layout. As long as Control keeps relative, the positioning is stable. The 5 affected snapshots (components-slider--*) Happo-render identically across page contexts because of this.

That said, if you want the cleaner shape, there's a deeper refactor:

Option A (minimal, no visual change) — drop ! from Tailwind, move overrides to inline style prop:

- <BaseUISlider.Track className="block !absolute w-full h-[1px] opacity-[0.24] rounded-none bg-gray-500" />
+ <BaseUISlider.Track style={{ position: 'absolute' }} className="block w-full h-[1px] opacity-[0.24] rounded-none bg-gray-500" />

Same pattern for Indicator + Thumb (Thumb gets style={{ translate: 'none', top: '-7px' }}). Keeps DOM structure → zero Happo risk.

Option B (structural) — nest Indicator + Thumbs inside Track to follow base-ui's recommended composition (Track > Indicator + Thumb). Track stays position: relative (its default), Indicator inherits height + uses base-ui's insetInlineStart/width, Thumbs absolute via base-ui's own inline style. Cleanest, but it changes DOM and risks re-opening Happo regressions we just stabilized.

Leaning toward Option A for this PR (small, no visual delta, addresses both your concerns for the public-facing element) and tracking Option B as a follow-up after the migration lands. Apply Option A? 👍 to confirm, or 🅱️/reply if you'd prefer to go straight to Option B (and accept a fresh Happo iteration).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I prefer option B, because inline style solution is unacceptable

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

🤖 Orchestrator agent (autonomous review-response)

Done — switched to Option B: Indicator + Marks + Thumbs are now children of Track. No !absolute overrides on Track or Indicator; no ![translate:none] or ![top] on Thumb. base-ui's defaults handle all positioning cleanly.

<BaseUISlider.Indicator className={indicatorClassName} />
{marks &&
markPositions.map((markValue, index) => (
<SliderMark
key={`mark-${markValue}`}
index={index}
markValue={markValue}
sliderValue={value}
positionPercent={
max === min ? 0 : ((markValue - min) / (max - min)) * 100
}
forceInactive={Boolean(disableTrackHighlight)}
/>
))}
{thumbValues.map((thumbValue, index) => {
const thumbId = index === 0 ? id : undefined
const label = formatValueLabel(thumbValue, index, tooltipFormat)

return (
<BaseUISlider.Thumb
Comment thread
vedrani marked this conversation as resolved.
Outdated
// eslint-disable-next-line react/no-array-index-key
key={`thumb-${index}`}
index={index}
id={thumbId}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

🤖 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

onFocus={onFocus}
onBlur={onBlur}
className={thumbClassName}
>
<SliderValueLabel
index={index}
value={thumbValue}
tooltip={isObserved ? tooltip : 'off'}
onRender={handleValueLabelOnRender}
yPlacement={isOnScreen ? 'top' : 'bottom'}
isOverlaped={isPartiallyOverlapped}
>
{label}
</SliderValueLabel>
</BaseUISlider.Thumb>
)
})}
{!isRange && thumbValues.length === 0 && (
Comment thread
dulishkovych marked this conversation as resolved.
Outdated
<BaseUISlider.Thumb
index={0}
id={id}
onFocus={onFocus}
onBlur={onBlur}
className={thumbClassName}
/>
)}
</BaseUISlider.Control>
</BaseUISlider.Root>
</div>
)
})
Expand Down
Loading
Loading