Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Slider] New Slider components and hook #373

Merged
merged 73 commits into from
Jul 2, 2024

Conversation

mj12albert
Copy link
Member

@mj12albert mj12albert commented Apr 29, 2024

Closes #216
Closes #76
Closes #77

Preview

https://deploy-preview-373--base-ui.netlify.app/base-ui/react-slider/

Major changes

  • Thumbs can no longer swap positions/be dragged past each other. The related disableSwap prop has been dropped as well.
  • A new Control subcomponent is added to represent the click/touch area of the slider. Previously this was combined with the root, but Slider.Root in the new API is expected to contain the Output element and a label as well
  • The rail slot/component previously referred to the full length of the slider. This is now renamed to instead be the Track
  • The previous track component referred to the filled portion of the slider "bar". This is now renamed to instead be the Indicator
  • The API does not expose the thumb's input element anymore, it's automatically wired to the corresponding thumb (a span by default), and allows for styling the thumb directly with :focus-visible
  • The Mark component/slot, and the marks prop will be dropped. Marks will be reintroduced with a new API in [Slider] Support for marks #462
  • The step prop defaults to 1 and can no longer be null.
  • The ValueLabel slot/component and related features have been dropped. This can be implemented using Tooltip as in this demo
  • The restricted values feature will be redesigned, and decoupled from any props related to the visual marks: [Slider] Arbitrarily restrict the possible values of the slider #412
  • The isRtl prop is replaced with direction: 'ltr' | 'rtl' for consistency
  • A new minStepsBetweenValues prop (default 0) is added to prevent thumb overlap
  • The scale prop is deprecated

Summary

  • All relevant tests from material-ui have been ported over, a few new ones were added
  • The render prop for the Thumb component is a special case that does not use standard utils, because we need to render a "hidden" <input> element. As a function it provides an additional inputProps argument, and it also supports the shorthand syntax. Documented here.
  • On the final location of the input - it's kept as a child of the thumb (for now) because we could solve [Slider] Issues with adding focus-visible styles to the thumb slot #77 without actually having to move the input

Demos

The first 4 are also in the docs

@mj12albert mj12albert added the component: slider This is the name of the generic UI component, not the React module! label Apr 29, 2024
@mj12albert mj12albert force-pushed the feat/slider-rewrite branch 13 times, most recently from dd496d9 to 366c478 Compare May 6, 2024 10:31
@colmtuite

This comment was marked as resolved.

@mj12albert mj12albert force-pushed the feat/slider-rewrite branch from 366c478 to b1707f7 Compare May 8, 2024 10:32
@mj12albert

This comment was marked as resolved.

@colmtuite

This comment was marked as resolved.

@colmtuite

This comment was marked as resolved.

@mbrookes

This comment was marked as resolved.

@colmtuite

This comment was marked as resolved.

@mbrookes

This comment was marked as resolved.

@mj12albert

This comment was marked as outdated.

@colmtuite

This comment was marked as resolved.

@mj12albert

This comment was marked as resolved.

@colmtuite

This comment was marked as resolved.

@mj12albert mj12albert force-pushed the feat/slider-rewrite branch 2 times, most recently from b4723a8 to 1c97949 Compare May 11, 2024 00:58
@mui-bot
Copy link

mui-bot commented May 11, 2024

Netlify deploy preview

https://deploy-preview-373--base-ui.netlify.app/

Generated by 🚫 dangerJS against 37059b8

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 25, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 25, 2024
@colmtuite
Copy link
Contributor

@mj12albert

  • When you focus a single thumb slider, VO announces “[label], group, [value], group”. I think it should announce “[label], slider, [value], group” or “[value], [label], slider, [label], group”?
  • In a range slider, when you move focus from the first thumb to the second thumb, VO announces “end range, [value], group”. I think it should announce “[value], end range, slider”.
  • What is the scale prop on Root?

@mj12albert
Copy link
Member Author

In a range slider, when you move focus from the first thumb to the second thumb, VO announces “end range, [value], group”. I think it should announce “[value], end range, slider”.

I've updated this so it announces "value" before "start/end range", it's announcing correctly as "slider" for me and not group though, I think this is just a VO quirk:

range.mov

When you focus a single thumb slider, VO announces “[label], group, [value], group”. I think it should announce “[label], slider, [value], group” or “[value], [label], slider, [label], group”?

You mean when VO focuses on the Root and not the Thumb right? I don't think it's possible to get it to announce "slider" unless it explicitly has the slider role (which is obviously incorrect). It also doesn't announce "group" like that for me, it could also be a VO issue:

single.mov

What is the scale prop on Root?

The scale prop enables a non-linear scale (a feature from Material UI): https://mui.com/material-ui/react-slider/#non-linear-scale
We could add a demo for this, unless you want to get rid of it? (Or are you just asking)

@colmtuite

@colmtuite
Copy link
Contributor

colmtuite commented Jun 28, 2024

@mj12albert Ok on the a11y stuff 👍

On scale, I was just asking what it was. I read the MUI docs just now and still not sure what it does? 😅

rootRef,
scale = Identity,
step = 1,
tabIndex,
Copy link
Member

Choose a reason for hiding this comment

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

tabIndex seems unused

Copy link
Member Author

Choose a reason for hiding this comment

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

@michaldudak tabIndex in the return value here gets put into context and is used by the Thumb like this:

it('can be removed from the tab sequence', () => {
render(<TestSlider tabIndex={-1} value={30} />);
expect(screen.getByRole('slider')).to.have.property('tabIndex', -1);
});

I suppose it could be passed straight from the SliderRoot component into the context without going through the hook, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Yup, if it's not used by the hook, it shouldn't go there.

packages/mui-base/src/Slider/Root/SliderRoot.types.ts Outdated Show resolved Hide resolved
packages/mui-base/src/Slider/Root/SliderRoot.types.ts Outdated Show resolved Hide resolved

const handleRootRef = useForkRef(rootRef, sliderRef);

const areValuesEqual = React.useCallback(
Copy link
Member

Choose a reason for hiding this comment

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

Is there any specific reason this is returned by the hook and not defined as a static function that takes two parameters to compare?

Copy link
Member Author

Choose a reason for hiding this comment

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

@michaldudak The 2nd parameter (the "old" value to be compared against) would be the internal valueState which is internal to useSliderRoot, I opted to pass this function through the hook instead of passing valueState as it seemed messier to pass 3 "values" through the hook - values, percentageValues, and valueState

packages/mui-base/src/Slider/SliderThumb/SliderThumb.tsx Outdated Show resolved Hide resolved
) => React.ReactElement)
| (React.ReactElement & { ref: React.Ref<Element> })
| keyof typeof defaultRenderFunctions;
}
Copy link
Member

Choose a reason for hiding this comment

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

It should be possible anyway with the standard useComponentRenderer, right?
render={(props) => <CustomThumb {...props}><SomeOtherComponent />{props.children}</SomeOtherComponent></CustomThumb>}.
I suppose it would just require some changes to the short form of the render prop (render={<CustomThumb><SomeOtherComponent /></CustomThumb}), to merge the children with internal children.

@mj12albert
Copy link
Member Author

mj12albert commented Jul 2, 2024

I read the MUI docs just now and still not sure what it does? 😅

Not sure if I can explain it better than the docs but let me try 😓 :

First set up a slider like this: (omitted subcomponents for simplicity)

<Slider.Root min={5} step={1} max={10} defaultValue={5} />

The possible values are: 5, 6, 7, 8,9, 10

Now we add the scale prop, which is a function that can transform the slider value.

<Slider.Root min={5} step={1} max={10} defaultValue={5} scale={(value) => 2 ** value } />

So now the default value of 5 will become 2^5 = 32
And the possible values of the slider become 2^5, 2^6, 2^7... 2^10 instead, for example to make a slider that lets you pick values among: 32kb, 64kb, 128kb, 256kb, 512kb like the Material UI demo

Does this make sense? Maybe it's the name scale that is unclear 🤔 @colmtuite

@mj12albert mj12albert force-pushed the feat/slider-rewrite branch from 8d1c24b to fbe5ef1 Compare July 2, 2024 05:35
@colmtuite
Copy link
Contributor

@mj12albert I understand now, thanks for explaining. Let's remove this functionality+prop for now.

@mj12albert mj12albert requested a review from michaldudak July 2, 2024 08:45
@mj12albert
Copy link
Member Author

@mj12albert I understand now, thanks for explaining. Let's remove this functionality+prop for now.

OK I've removed it

@colmtuite colmtuite merged commit 55ba1a8 into mui:master Jul 2, 2024
18 checks passed
```jsx
<Slider.Thumb
render={(props, inputProps) => {
const { children, ...rest } = props;
Copy link
Member

@oliviertassinari oliviertassinari Jul 27, 2024

Choose a reason for hiding this comment

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

We might want to change the naming convention in the codebase for this, but for now since it's one of the only variations we have in the codebase, it's simpler to stick to it (it's simpler to do a mass codebase update all at once if we want to change this).

Suggested change
const { children, ...rest } = props;
const { children, ...other } = props;

Done in 89821a4

(I found this variation from reading the docs in MUI X Charts initially, I then did a quick search in VS Code where my environment has all the codebase of the organization in the VS Code workspace, to see the codebase patterns, and I found this in Base UI too).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: slider This is the name of the generic UI component, not the React module!
Projects
None yet
7 participants