[Slider][base] Improve logic for displaying value label #35479

ZeeshanTamboli · 2022-12-14T10:54:26Z

Breaking Changes for MUI Base:

The valueLabelDisplay prop is removed from SliderUnstyled. It was not working as intended in SliderUnstyled (See [docs] Customize valueLabel in SliderUnstyled with tailwind #35398). You can instead provide a valueLabel slot with the slots prop API to show the value label:

- <SliderUnstyled valueLabelDisplay="on" />
+ <SliderUnstyled slots={{ valueLabel: SliderValueLabel }} />

The following demo shows how to show value label when it is hovered upon the thumb: https://deploy-preview-35479--material-ui.netlify.app/base/react-slider/#value-label

The following classes are removed from sliderUnstyledClasses since they are not needed for value label:

- valueLabel
- valueLabelOpen
- valueLabelCircle
- valueLabelLabel

You can define your own classNames in the custom value label component and target them in CSS.

The SliderValueLabelUnstyled component is removed from SliderUnstyled. You can provide your own custom component for the value label.
The Value label component hierarchy structure is changed to avoid using React.cloneElement in value label. The value label is now inside the Thumb slot - Thumb -> Input, ValueLabel.

I have followed (at least) the PR section of the contributing guide.

There are two things in this PR:

Move the logic of valueLabelDisplay prop to Material UI. To do this, we migrate the Material UI Slider to use useSlider hook. This has the advantage of how the Slider's component structure is rendered by Material UI and also we are able to have the valueLabelDisplay logic controlled which is not needed in MUI Base due to the point below.
Remove the SliderLabelValueUnstyled component from MUI Base and show the value label by simply hovering on the thumb in SliderUnstyled. Add demos to showcase it (preview below). To avoid breaking changes in Material UI, we keep the same component structure like SliderLabelValueUnstyled for the value label.

Preview: https://deploy-preview-35479--material-ui.netlify.app/base/react-slider/#value-label

Closes #35398

mui-bot · 2022-12-14T10:57:08Z

	Messages
📖	Netlify deploy preview: https://deploy-preview-35479--material-ui.netlify.app/

@material-ui/core: parsed: -0.16% 😍, gzip: -0.19% 😍
@material-ui/unstyled: parsed: -0.66% 😍, gzip: -0.50% 😍

Details of bundle changes

Generated by 🚫 dangerJS against 886803e

…dd-demo

docs/data/base/components/slider/slider.md

mnajdova · 2022-12-30T13:18:35Z

docs/pages/material-ui/api/slider.json

@@ -60,7 +60,7 @@
    "slotProps": {
      "type": {
        "name": "shape",
-        "description": "{ input?: func<br>&#124;&nbsp;object, mark?: func<br>&#124;&nbsp;object, markLabel?: func<br>&#124;&nbsp;object, rail?: func<br>&#124;&nbsp;object, root?: func<br>&#124;&nbsp;object, thumb?: func<br>&#124;&nbsp;object, track?: func<br>&#124;&nbsp;object, valueLabel?: func<br>&#124;&nbsp;{ children?: element, className?: string, open?: bool, style?: object, value?: number, valueLabelDisplay?: 'auto'<br>&#124;&nbsp;'off'<br>&#124;&nbsp;'on' } }"
+        "description": "{ input?: func<br>&#124;&nbsp;object, mark?: func<br>&#124;&nbsp;object, markLabel?: func<br>&#124;&nbsp;object, rail?: func<br>&#124;&nbsp;object, root?: func<br>&#124;&nbsp;object, thumb?: func<br>&#124;&nbsp;object, track?: func<br>&#124;&nbsp;object, valueLabel?: any<br>&#124;&nbsp;func }"


There shouldn't change here, the Material UI API's stays intact.

It is because of this change in the slotProps type in SliderUnstyled.d.ts:

--- a/packages/mui-base/src/SliderUnstyled/SliderUnstyled.types.ts +++ b/packages/mui-base/src/SliderUnstyled/SliderUnstyled.types.ts @@ -171,7 +171,7 @@ export interface SliderUnstyledOwnProps { SliderUnstyledOwnerState >; valueLabel?: SlotComponentProps< - typeof SliderValueLabelUnstyled, + React.ElementType, SliderUnstyledComponentsPropsOverrides, SliderUnstyledOwnerState >;

SliderValueLabelUnstyled component is removed and the Material UI Slider types extend the SliderUnstyled types.
Is there any way? There is one DeepOmit utility: https://github.com/ts-essentials/ts-essentials#deepomit.

mnajdova · 2022-12-30T13:19:51Z

packages/mui-base/src/SliderUnstyled/SliderUnstyled.js

@@ -155,7 +149,7 @@ const SliderUnstyled = React.forwardRef(function SliderUnstyled(props, ref) {
    ownerState,
  });

-  const ValueLabel = slots.valueLabel ?? SliderValueLabelUnstyled;
+  const ValueLabel = slots.valueLabel;


Will this simplify the implementation:

Suggested change

const ValueLabel = slots.valueLabel;

const ValueLabel = slots.valueLabel ?? Forward;

The ValueLabel is now inside the Thumb slot in the JSX structure so it's not needed to Forward the children.

mnajdova · 2022-12-30T13:22:29Z

packages/mui-base/src/SliderUnstyled/SliderUnstyled.js

+              value={values[index]}
+              {...inputProps}
+            />
+            {ValueLabel ? (


This would change the structure for the Material UI components. What was the reason for changing it? The ValueLabel should wrap the Thumb.

This would change the structure for the Material UI components.

It won't change since SliderUnstyled is not used in Material UI's Slider component now. We use the useSlider hook and keep the JSX structure intact like the current SliderUnstyled in production. The useSlider hook is used to implement the valueLabelDisplay logic in Material UI Slider.

What was the reason for changing it?

If you inspect the generated HTML DOM structure, the value label is actually inside the thumb slot due to this code in the SliderValueLabelUnstyled. It means that even though the ValueLabel wraps the thumb in the JSX structure, in the final generated HTML the thumb wraps the value label due to React.cloneElement.

Thus I changed the structure only in SliderUnstyled and avoided the need to provide the complicated React.cloneElement implementation for custom ValueLabel component.

This is how Joy UI's Slider is also implemented. See https://github.com/mui/material-ui/blob/master/packages/mui-joy/src/Slider/Slider.tsx#L608-L639.

I would also recommend to read the PR description and also review Slider.js file in packages/@mui-material in this PR if you haven't.

For easing up the review I would recommend splitting this in two pull requests. We can go first with replacing the SliderUnstyled with useSlider in @mui/material. It's important we don't have regressions with this change. After this one, we can safely do any changes we want to the SliderUnstyled, making sure it won't affect the Material UI's version. We aim to have each PR fix one particular problem, this would also make the review much easier.

I would also recommend to read the PR description and also review Slider.js file in packages/@mui-material in this PR if you haven't.

I focused only on the changes inside SliderUnstyled in the initial review, haven't checked the other changes, I was only looking for potential breaking changes we may do.

It makes sense. I will split it into two PRs.

packages/mui-base/src/SliderUnstyled/SliderUnstyled.js

Co-authored-by: Marija Najdova <mnajdova@gmail.com> Signed-off-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com>

…dd-demo

…s SliderValueLabel to avoid breaking change

ZeeshanTamboli · 2023-01-09T10:22:16Z

As discussed in #35479 (comment), splitting this PR into two PRs. So closing in favor of #35770 and ZeeshanTamboli#2.

remove valueLabelDisplay from SliderUnstyled

894baea

ZeeshanTamboli added component: slider This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base labels Dec 14, 2022

fix tests

8f6a076

ZeeshanTamboli changed the title ~~[SliderUnstyled] Improve logic for displaying value label~~ [Slider][Base] Improve logic for displaying value label Dec 15, 2022

ZeeshanTamboli added 17 commits December 16, 2022 12:56

remove SliderValueLabelUnstyled component

beb8925

show value label on hover

4474ccb

add TODO for v6

b1aeb3e

use useSlider hook in Material-UI slider

a12991c

fix code

04ad9aa

Merge branch 'master' into issue/35398-remove-valueLabelDisplay-and-a…

0a45706

…dd-demo

rename Slider value label component and migrate to TypeScript

3b85d6a

yarn proptypes

1a77944

yarn docs:api

f5cd6c2

yarn docs:typescript:formatted

0063d08

yarn docs:typescript:formatted

052b617

destructure props

c334e6f

fix argos by passing ownerState to root and track slots

e6f40c6

remove classes

47a9a9e

Merge branch 'master' into issue/35398-remove-valueLabelDisplay-and-a…

62f2aec

…dd-demo

Change structure of value label in SliderUnstyled

bf03a98

fix dem

86b62fc

ZeeshanTamboli added breaking change bug 🐛 Something doesn't work labels Dec 23, 2022

ZeeshanTamboli marked this pull request as ready for review December 23, 2022 14:27

ZeeshanTamboli requested a review from a team December 23, 2022 14:31

ZeeshanTamboli added the docs Improvements or additions to the documentation label Dec 23, 2022

ZeeshanTamboli mentioned this pull request Dec 23, 2022

[Slider][base] Convert code to TypeScript #35445

Merged

1 task

ZeeshanTamboli changed the title ~~[Slider][Base] Improve logic for displaying value label~~ [Slider][base] Improve logic for displaying value label Dec 26, 2022

mnajdova reviewed Dec 30, 2022

View reviewed changes

ZeeshanTamboli and others added 9 commits December 30, 2022 19:28

Update docs/data/base/components/slider/slider.md

289bd69

Co-authored-by: Marija Najdova <mnajdova@gmail.com> Signed-off-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com>

Update docs/data/base/components/slider/slider.md

b582d01

Co-authored-by: Marija Najdova <mnajdova@gmail.com> Signed-off-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com>

Update docs/data/base/components/slider/slider.md

aca075a

Co-authored-by: Marija Najdova <mnajdova@gmail.com> Signed-off-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com>

yarn prettier

3543f3b

remove unnecessary valueLabelProps className

01e215d

Merge branch 'master' into issue/35398-remove-valueLabelDisplay-and-a…

76e27b4

…dd-demo

rename SliderValueLabel component and export StyledSliderValueLabel a…

1a0e1ff

…s SliderValueLabel to avoid breaking change

remove not needed SliderValueLabelProps from SliderUnstyled types

4de0741

export SliderValueLabelProps

886803e

ZeeshanTamboli requested a review from mnajdova January 3, 2023 10:15

ZeeshanTamboli mentioned this pull request Jan 9, 2023

[Slider] Replace SliderUnstyled with useSlider hook #35770

Merged

1 task

ZeeshanTamboli closed this Jan 9, 2023

ZeeshanTamboli mentioned this pull request Jan 12, 2023

[SliderUnstyled] Improved logic for displaying the value label #35805

Merged

1 task

ZeeshanTamboli deleted the issue/35398-remove-valueLabelDisplay-and-add-demo branch February 6, 2023 11:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Slider][base] Improve logic for displaying value label #35479

[Slider][base] Improve logic for displaying value label #35479

ZeeshanTamboli commented Dec 14, 2022 •

edited

mui-bot commented Dec 14, 2022 •

edited

mnajdova Dec 30, 2022 •

edited

ZeeshanTamboli Jan 3, 2023

mnajdova Dec 30, 2022

ZeeshanTamboli Jan 3, 2023

mnajdova Dec 30, 2022

ZeeshanTamboli Jan 3, 2023 •

edited

mnajdova Jan 3, 2023

ZeeshanTamboli Jan 7, 2023

ZeeshanTamboli commented Jan 9, 2023

	const ValueLabel = slots.valueLabel;
	const ValueLabel = slots.valueLabel ?? Forward;

[Slider][base] Improve logic for displaying value label #35479

[Slider][base] Improve logic for displaying value label #35479

Conversation

ZeeshanTamboli commented Dec 14, 2022 • edited

Breaking Changes for MUI Base:

mui-bot commented Dec 14, 2022 • edited

mnajdova Dec 30, 2022 • edited

Choose a reason for hiding this comment

ZeeshanTamboli Jan 3, 2023

Choose a reason for hiding this comment

mnajdova Dec 30, 2022

Choose a reason for hiding this comment

ZeeshanTamboli Jan 3, 2023

Choose a reason for hiding this comment

mnajdova Dec 30, 2022

Choose a reason for hiding this comment

ZeeshanTamboli Jan 3, 2023 • edited

Choose a reason for hiding this comment

mnajdova Jan 3, 2023

Choose a reason for hiding this comment

ZeeshanTamboli Jan 7, 2023

Choose a reason for hiding this comment

ZeeshanTamboli commented Jan 9, 2023

ZeeshanTamboli commented Dec 14, 2022 •

edited

mui-bot commented Dec 14, 2022 •

edited

mnajdova Dec 30, 2022 •

edited

ZeeshanTamboli Jan 3, 2023 •

edited