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][base] Improve logic for displaying value label #35479
[Slider][base] Improve logic for displaying value label #35479
Conversation
@material-ui/core: parsed: -0.16% 😍, gzip: -0.19% 😍 |
@@ -60,7 +60,7 @@ | |||
"slotProps": { | |||
"type": { | |||
"name": "shape", | |||
"description": "{ input?: func<br>| object, mark?: func<br>| object, markLabel?: func<br>| object, rail?: func<br>| object, root?: func<br>| object, thumb?: func<br>| object, track?: func<br>| object, valueLabel?: func<br>| { children?: element, className?: string, open?: bool, style?: object, value?: number, valueLabelDisplay?: 'auto'<br>| 'off'<br>| 'on' } }" | |||
"description": "{ input?: func<br>| object, mark?: func<br>| object, markLabel?: func<br>| object, rail?: func<br>| object, root?: func<br>| object, thumb?: func<br>| object, track?: func<br>| object, valueLabel?: any<br>| func }" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There shouldn't change here, the Material UI API's stays intact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -155,7 +149,7 @@ const SliderUnstyled = React.forwardRef(function SliderUnstyled(props, ref) { | |||
ownerState, | |||
}); | |||
|
|||
const ValueLabel = slots.valueLabel ?? SliderValueLabelUnstyled; | |||
const ValueLabel = slots.valueLabel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this simplify the implementation:
const ValueLabel = slots.valueLabel; | |
const ValueLabel = slots.valueLabel ?? Forward; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ValueLabel is now inside the Thumb slot in the JSX structure so it's not needed to Forward the children.
value={values[index]} | ||
{...inputProps} | ||
/> | ||
{ValueLabel ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would change the structure for the Material UI components. What was the reason for changing it? The ValueLabel should wrap the Thumb.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense. I will split it into two PRs.
Co-authored-by: Marija Najdova <mnajdova@gmail.com> Signed-off-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com>
Co-authored-by: Marija Najdova <mnajdova@gmail.com> Signed-off-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com>
Co-authored-by: Marija Najdova <mnajdova@gmail.com> Signed-off-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com>
…s SliderValueLabel to avoid breaking change
As discussed in #35479 (comment), splitting this PR into two PRs. So closing in favor of #35770 and ZeeshanTamboli#2. |
Breaking Changes for MUI Base:
valueLabelDisplay
prop is removed fromSliderUnstyled
. It was not working as intended inSliderUnstyled
(See [docs] Customize valueLabel in SliderUnstyled with tailwind #35398). You can instead provide avalueLabel
slot with theslots
prop API to show the value label: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
sliderUnstyledClasses
since they are not needed for value label: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
.There are two things in this PR:
valueLabelDisplay
prop to Material UI. To do this, we migrate the Material UI Slider to useuseSlider
hook. This has the advantage of how the Slider's component structure is rendered by Material UI and also we are able to have thevalueLabelDisplay
logic controlled which is not needed in MUI Base due to the point below.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 likeSliderLabelValueUnstyled
for the value label.Preview: https://deploy-preview-35479--material-ui.netlify.app/base/react-slider/#value-label
Closes #35398