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

[SliderUnstyled] Use useSlotProps #33132

Merged
merged 12 commits into from Jun 24, 2022

Conversation

michaldudak
Copy link
Member

Prepare slots' props with the useSlotProps hook.

Additionally, extended the useSlotProps to merge the style props instead of overwriting them

@michaldudak michaldudak added component: slider This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base labels Jun 13, 2022
@@ -11,6 +11,7 @@ import {

export type SliderUnstyledOwnerState = SliderUnstyledProps & {
disabled: boolean;
focusVisible: boolean;
Copy link
Member Author

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

@mui-bot
Copy link

mui-bot commented Jun 13, 2022

Details of bundle changes

@material-ui/core: parsed: +0.10% , gzip: +0.07%
@material-ui/unstyled: parsed: +0.50% , gzip: +0.12%

Generated by 🚫 dangerJS against 8534f0a

@@ -59,7 +59,6 @@ const SliderUnstyled = React.forwardRef(function SliderUnstyled(props, ref) {
name,
onChange,
onChangeCommitted,
onMouseDown,
Copy link
Member Author

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

@michaldudak michaldudak marked this pull request as draft June 13, 2022 10:06
@@ -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,
Copy link
Member

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;
Copy link
Member

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.

Copy link
Member Author

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.

@michaldudak michaldudak marked this pull request as ready for review June 13, 2022 11:02
className={clsx(classes.thumb, thumbProps.className, {
[classes.active]: active === index,
[classes.focusVisible]: focusVisible === index,
[classes.focusVisible]: focusedThumbIndex === index,
Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

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 👍

Copy link
Member

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.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 16, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 21, 2022
@michaldudak
Copy link
Member Author

@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 }) => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
})<{ 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

All right, you motivated me to improve the return type of useSlotProps :)
Now, when we're sure that a React component is used (not a host one), the ownerState field will be present.

See c99ea5f

cc @mnajdova

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.
Copy link
Member

@mnajdova mnajdova left a 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<
Copy link
Member

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 });
Copy link
Member

Choose a reason for hiding this comment

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

👌 very good tests!

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 Great job!

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 24, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 24, 2022
@michaldudak michaldudak merged commit c016a75 into mui:master Jun 24, 2022
@michaldudak michaldudak deleted the componentsProps/slider branch June 24, 2022 10:22
daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: slider This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants