[Slider] Add support for Arrow Down/Up + Shift and Page Up/Down keys #40676

mnajdova · 2024-01-18T13:06:33Z

Some design decisions:

added pageStep prop that defines the size of the step when using these keys - the default value is 10
if step = null - this means that the values are restricticed based on the marks defined - in this case the Page Up/Down behaves the same way as Arrow Up / Down as there is no determistic way of how much it should move
added note on the discrete sliders to specify the pageStep value with a value dividable with the step prop.

Preview:

mnajdova · 2024-01-18T13:08:22Z

packages/mui-base/src/useSlider/useSlider.ts

@@ -296,6 +297,89 @@ export function useSlider(parameters: UseSliderParameters): UseSliderReturnValue
      otherHandlers?.onBlur?.(event);
    };

+  const changeValue = (event: React.KeyboardEvent | React.ChangeEvent, valueInput: number) => {


I've extracted the previous createHandleHiddenInputChange logic in a new method that can be re-used in two places now. The only difference is how we read the value (from the input target, or calculating it in case the PageUp/PageDown arrows are used.

mui-bot · 2024-01-18T13:09:20Z

Netlify deploy preview

@material-ui/core: parsed: +0.09% , gzip: +0.12%
@material-ui/unstyled: parsed: +0.32% , gzip: +0.32%
@mui/material-next: parsed: +0.14% , gzip: +0.15%
@mui/joy: parsed: +0.10% , gzip: +0.12%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against bb84465

mnajdova · 2024-01-18T14:44:24Z

docs/data/base/components/slider/DiscreteSlider.tsx

@@ -9,6 +9,7 @@ export default function DiscreteSlider() {
        aria-label="Temperature"
        defaultValue={30}
        getAriaValueText={valuetext}
+        pageStep={3}


Using a smaller pageStep on the discrete sliders looks better.

colmtuite · 2024-01-18T15:54:43Z

It seems that shift+right is incrementing the slider value by 11 rather than 10. Since pageStep = step * pageStep when step is also provided, I'm guessing this is because step is set to 1 by default?

To avoid this, I think I'd be in favour of changing how that works to one of these options:

Set pageStep to 10 and make it non-configurable.
Make pageStep configurable, but let step and pageStep be independent, so one can't affect the other.

mnajdova · 2024-01-18T16:30:24Z

Thanks for checking this Colm!

It seems that shift+right is incrementing the slider value by 11 rather than 10. Since pageStep = step * pageStep when step is also provided, I'm guessing this is because step is set to 1 by default?

It should be incremented by 10, pageStep = step * pageStep - this should end up being 10 still. Do you have an example of this? I haven't caught this with the tests.

To avoid this, I think I'd be in favour of changing how that works to one of these options:

Set pageStep to 10 and make it non-configurable.

Make pageStep configurable, but let step and pageStep be independent, so one can't affect the other.

On 2. - this is what I initially thought the API should look like. The reason why I ended up multiplying these two values was because it made more sense in the cases when step is defined. If the step is let's say 10, by default the Shift + Arrow Up will increment the value by the same amount as just using Arrow Up. Multiplying them means that Shift + Arrow Up/Down will always increment/decrement the value pageStep times more than the step. I think that any option we choose would mean that people will have to adjust these values, so yeah, maybe it's better to go with the simplest API.

cc @DiegoAndai @michaldudak what are your thoughts? Considering this will affect both Base UI and Material UI?

colmtuite · 2024-01-18T16:44:23Z

lol you're right, math is hard 😄

Video attached of the first docs example in the deploy preview. The value is initially set to '0'. Then I press Shift+Right, and it increments by '11'.

Screen.Recording.2024-01-18.at.16.32.48.mp4

If the step is let's say 10, by default the Shift + Arrow Up will increment the value by the same amount as just using Arrow Up.

I'm not sure this is necessarily an issue, especially since the user configures the values themselves. I think having the two step increments dependent on each other is more of an issue, because it makes certain configurations more complicated. Let's say I want step="2" and pageStep="10", I have actually need to set pageStep="5", to get a pageStep value of 10. So I need to do some math to get there, and as I just demonstrated in my previous comment, math is hard 🤣

So I think the issue with the dependency between the two props is that it's not inherent or explicitly communicated. It's kind of just magic, which I think will probably be confusing.

I think if both props are independent, we just let the user set their own values and it's up to them how the slider works.

mnajdova · 2024-01-19T09:38:42Z

So I think the issue with the dependency between the two props is that it's not inherent or explicitly communicated. It's kind of just magic, which I think will probably be confusing.

I think if both props are independent, we just let the user set their own values and it's up to them how the slider works.

Yeah, ok fair enough. I will update the PR and check the issue from the video. Maybe the input type="range" increments on each Arrow Right by one too.

DiegoAndai · 2024-01-19T11:21:25Z

Regarding the step and pageStep API, what makes sense to me is to:

Have a sensible pageStep default: step * 10
If the pageStep prop is provided, it overrides the default and is independent.

So for example:

<Slider /> -> step = 1, pageStep = 10
<Slider step={2} /> -> step = 2, pageStep = 20
<Slider step={2} pageStep={10} /> -> step = 2, pageStep = 10

mnajdova · 2024-01-19T15:31:13Z

Regarding the step and pageStep API, what makes sense to me is to:

Have a sensible pageStep default: step * 10
If the pageStep prop is provided, it overrides the default and is independent.

I am not 100% sure about this. It works for the examples you provided, but for e.g. if we have values from 1 to 100, with step of 10, it will fail again. It would be simpler to just say, by default the page step is 10, and for any other value, override it.

colmtuite · 2024-01-19T15:51:21Z

-> step = 2, pageStep = 20

I misread this part of @DiegoAndai's comment when I put my thumbs up. This is an example of the magic I was referring to above. Nothing tells me about the dependency between the two props, one prop just updates the other without me telling it to work like that.

Imagine you set pageStep to 5, you check it and it works great. Then you set step to 2. You test it, and all of a sudden your pageStep value is wrong...

if we have values from 1 to 100, with step of 10, it will fail again

It also causes problems/confusion like in this example from @mnajdova.

It would be simpler to just say, by default the page step is 10, and for any other value, override it.

imo this is the simplest and best API. step defaults to 1, but you can change it to whatever you want. pageStep defaults to 10, but you can change it to whatever you want. If you choose to set both props to matching values, fine.

mnajdova · 2024-01-22T10:08:10Z

Cool, the API is updated based on the conclusion (I've also updated the PR description). We can move into reviewing the logic next.

colmtuite

Tested it in the docs and LGTM 👍

packages/mui-base/src/useSlider/useSlider.types.ts

packages/mui-base/src/Slider/Slider.test.tsx

mnajdova added 4 commits January 17, 2024 16:25

wip

21f339d

fixes, types

c2fe70b

fix & tests

1343967

add test for PageUp/PageDown

bc82315

mnajdova added new feature New feature or request component: slider This is the name of the generic UI component, not the React module! labels Jan 18, 2024

mnajdova commented Jan 18, 2024

View reviewed changes

mnajdova added 2 commits January 18, 2024 15:40

lint

027f5cb

improve demos

1bdabcf

mnajdova commented Jan 18, 2024

View reviewed changes

mnajdova marked this pull request as ready for review January 18, 2024 14:44

mnajdova requested review from colmtuite and michaldudak January 18, 2024 14:44

mnajdova added 2 commits January 19, 2024 11:28

code review

571488c

Update prop description

c5bb806

mnajdova added 4 commits January 22, 2024 13:25

Merge branch 'master' into slider/support-page-up-down

e54d4f5

docs update

e1bcd8f

Material UI docs update

e44aaeb

Joy UI's docs

f50ce39

colmtuite approved these changes Jan 22, 2024

View reviewed changes

michaldudak reviewed Jan 24, 2024

View reviewed changes

packages/mui-base/src/useSlider/useSlider.types.ts Outdated Show resolved Hide resolved

mnajdova added 2 commits February 1, 2024 18:47

Merge branch 'master' into slider/support-page-up-down

78ef4e4

rename to shiftStep

29ed52b

michaldudak reviewed Feb 2, 2024

View reviewed changes

packages/mui-base/src/Slider/Slider.test.tsx Show resolved Hide resolved

mnajdova added 2 commits February 2, 2024 12:24

lint issue

fd80bb5

Add Shift + Arrow Down/Arrow Up test

bb84465

mnajdova merged commit 0086c0e into mui:master Feb 5, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Slider] Add support for Arrow Down/Up + Shift and Page Up/Down keys #40676

[Slider] Add support for Arrow Down/Up + Shift and Page Up/Down keys #40676

mnajdova commented Jan 18, 2024 •

edited

Loading

mnajdova Jan 18, 2024

mui-bot commented Jan 18, 2024 •

edited

Loading

mnajdova Jan 18, 2024

colmtuite commented Jan 18, 2024

mnajdova commented Jan 18, 2024

colmtuite commented Jan 18, 2024 •

edited

Loading

mnajdova commented Jan 19, 2024

DiegoAndai commented Jan 19, 2024 •

edited

Loading

mnajdova commented Jan 19, 2024

colmtuite commented Jan 19, 2024 •

edited

Loading

mnajdova commented Jan 22, 2024

colmtuite left a comment

[Slider] Add support for Arrow Down/Up + Shift and Page Up/Down keys #40676

[Slider] Add support for Arrow Down/Up + Shift and Page Up/Down keys #40676

Conversation

mnajdova commented Jan 18, 2024 • edited Loading

mnajdova Jan 18, 2024

Choose a reason for hiding this comment

mui-bot commented Jan 18, 2024 • edited Loading

Netlify deploy preview

Bundle size report

mnajdova Jan 18, 2024

Choose a reason for hiding this comment

colmtuite commented Jan 18, 2024

mnajdova commented Jan 18, 2024

colmtuite commented Jan 18, 2024 • edited Loading

mnajdova commented Jan 19, 2024

DiegoAndai commented Jan 19, 2024 • edited Loading

mnajdova commented Jan 19, 2024

colmtuite commented Jan 19, 2024 • edited Loading

mnajdova commented Jan 22, 2024

colmtuite left a comment

Choose a reason for hiding this comment

mnajdova commented Jan 18, 2024 •

edited

Loading

mui-bot commented Jan 18, 2024 •

edited

Loading

colmtuite commented Jan 18, 2024 •

edited

Loading

DiegoAndai commented Jan 19, 2024 •

edited

Loading

colmtuite commented Jan 19, 2024 •

edited

Loading