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
[SliderUnstyled] Use useSlotProps #33132
Conversation
@@ -11,6 +11,7 @@ import { | |||
|
|||
export type SliderUnstyledOwnerState = SliderUnstyledProps & { | |||
disabled: boolean; | |||
focusVisible: boolean; |
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 was missing in ownerState
@material-ui/core: parsed: +0.10% , gzip: +0.07% |
@@ -59,7 +59,6 @@ const SliderUnstyled = React.forwardRef(function SliderUnstyled(props, ref) { | |||
name, | |||
onChange, | |||
onChangeCommitted, | |||
onMouseDown, |
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 doesn't have to be specified explicitly anymore, as all extra event handlers from props are passed into the getRootProps
@@ -234,7 +266,6 @@ const SliderUnstyled = React.forwardRef(function SliderUnstyled(props, ref) { | |||
<Thumb | |||
data-index={index} | |||
{...thumbProps} | |||
{...getThumbProps()} | |||
className={clsx(classes.thumb, thumbProps.className, { | |||
[classes.active]: active === index, | |||
[classes.focusVisible]: focusVisible === index, |
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.
Take a look at what should be the focusVisible
owner state for the thumb component.
@@ -111,50 +110,83 @@ const SliderUnstyled = React.forwardRef(function SliderUnstyled(props, ref) { | |||
|
|||
ownerState.marked = marks.length > 0 && marks.some((mark) => mark.label); | |||
ownerState.dragging = dragging; | |||
ownerState.focusVisible = focusVisible; |
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 may be misleading, as the focusVisible
state in the Slider is the index of the focused 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.
It is misleading indeed. I'd rename it to focusedThumbIndex
.
className={clsx(classes.thumb, thumbProps.className, { | ||
[classes.active]: active === index, | ||
[classes.focusVisible]: focusVisible === index, | ||
[classes.focusVisible]: focusedThumbIndex === index, |
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 means in the styles/classes people would need to apply the focus visible classes if data-index === ownerState.focusedThumbIndex
. I think this is super confusing, I would rather just update the ownerState
of the thumb component to have the focusedVisible
if the condition is met.
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.
Interesting point.
So far, in all components, ownerState
was exactly the same in all slots (it's an owner state, not a slot one). I'd like to keep it this way.
We can either provide this information to the Thumb directly as a prop, or add a function to the ownerState: isThumbFocusVisible(index: number): boolean
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.
I had the same doubt as it is not really an ownerState
. Adding a new prop sounds great 👍
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.
👍 Had the same doubt when I was implementing the Slider for Joy.
@siriwatknp I had to make significant changes in Joy's Slider. Could you please review them? |
@@ -69,8 +65,8 @@ const SliderRoot = styled('span', { | |||
name: 'JoySlider', | |||
slot: 'Root', | |||
overridesResolver: (props, styles) => styles.root, | |||
})<{ ownerState: SliderProps }>(({ theme, ownerState }) => { | |||
const getColorVariables = sliderColorVariables({ theme, ownerState }); | |||
})<{ ownerState?: SliderOwnerState }>(({ theme, ownerState }) => { |
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.
})<{ ownerState?: SliderOwnerState }>(({ theme, ownerState }) => { | |
})<{ ownerState: SliderOwnerState }>(({ theme, ownerState }) => { |
I think we should not use optional here. This will make sure that ownerState
is passed as a prop in the component.
We have found an issue where ownerState
is missing. TypeScript can catch this bug effectively.
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.
Make useSlotProps (and the underlying appendOwnerState) aware of the type of element on which the props should be applied. Based on this, ownerState will either be present or undefined in the resulting type.
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.
I went through all changes and they look good, however, I could recommend splitting the PR into two, so that we can have the changes of the useSlotProps
as a separate commit.
* This resolves to the provided OwnerState for React components and `undefined` for host components. | ||
* Falls back to `OwnerState | undefined` when the exact type can't be determined in development time. | ||
*/ | ||
type OwnerStateWhenApplicable< |
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.
🏆 Nice! :))
import * as React from 'react'; | ||
import appendOwnerState from './appendOwnerState'; | ||
|
||
const divProps = appendOwnerState('div', { otherProp: true }, { ownerStateProps: true }); |
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.
👌 very good tests!
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.
👍 Great job!
Prepare slots' props with the useSlotProps hook.
Additionally, extended the useSlotProps to merge the
style
props instead of overwriting them