-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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] Improve TS definition of unstyled #26642
Conversation
@eps1lon should we add separate |
Sounds good! |
@eps1lon added, I just copied the script. Maybe I should have added the test inside the |
root?: { | ||
as: React.ElementType; | ||
styleProps?: Omit<SliderUnstyledTypeMap<P, D>['props'], 'components' | 'componentsProps'>; | ||
as?: React.ElementType; | ||
styleProps?: Omit<SliderUnstyledTypeMap<P, D>['props'], 'components' | 'componentsProps'> & | ||
SliderStylePropsOverrides; | ||
}; |
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.
@mnajdova Are we sure about these types? I'm lost when I look at them. In MUI-X @dtassone has used any
root?: { | |
as: React.ElementType; | |
styleProps?: Omit<SliderUnstyledTypeMap<P, D>['props'], 'components' | 'componentsProps'>; | |
as?: React.ElementType; | |
styleProps?: Omit<SliderUnstyledTypeMap<P, D>['props'], 'components' | 'componentsProps'> & | |
SliderStylePropsOverrides; | |
}; | |
root?: any; |
which might be too loose, but seems still better.
I mean, for instance, if I want to add an extra data-
attribute it fails:
import * as React from 'react';
import SliderUnstyled from '@material-ui/unstyled/SliderUnstyled';
export default function UnstyledSlider() {
return (
<SliderUnstyled componentsProps={{ root: { 'data-foo': 'bar' } }} defaultValue={10} />
);
}
On styleProps
specifically, I also don't see why we use SliderUnstyledTypeMap
. As far as I know, the correlation between the props and what's available in this prop is "light". For instance, in the slider, we have an extra marked
property:
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'm in favor of using specific types instead of any
whenever possible. This way developers get prop validation for free from Typescript (this can be especially important if a component with mandatory props is used in a slot).
The data-foo
you mentioned is a bit of an edge case as data attributes are not specified in any HTMLAttributes
. To use them in a type-safe way, module augmentation should be used. However, IMO this should be the only case where developers should resort to module augmentation when providing componentsProps
.
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.
@michaldudak data-*
, tabindex
, aria-*
, they all fail. We don't accept any other props than as
and styleProps
. The issue for me is that we don't have any easy way to know what are types accepted. A developer could use components
to render something completely different.
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.
In this case yes, they'll fail without module augmentation. But we could use what I made in https://github.com/mui-org/material-ui/blob/d587b1eab467739a2a143f0cd49bfc10cc35ba74/packages/material-ui-unstyled/src/SwitchUnstyled/SwitchUnstyled.tsx - when you pass in a component/intrinsic element in components
prop, the corresponding componentsProps
expects props for this exact component/element. data-*
attributes obviously aren't supported this way, but all the others are (including aria-*
)
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.
when you pass in a component/intrinsic element in components prop, the corresponding componentsProps
@michaldudak This approach seems to have potential 👍 .
Should we open a new issue to keep track of this problem?
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.
Sure thing: #26810
After doing some stress test for creating styled slider with TS I faced some issues that are fixes in this PR:
styleProps
as
prop option on thecomponentsProps.root