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

[Joy] Use native html for root slot #33773

Closed
wants to merge 11 commits into from

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Aug 3, 2022

Motivation

Our Input and Select requires a div for creating the layout because we want to make it easy to add styles and start/end decorators. Conventionally, all the props are spread to the div (defined as the root element), which makes the DX sometimes really bad:

<Input componentsProps={{ input: { aria-label: 'Search' } }} />
<Input componentsProps={{ input: { component: 'textarea' } }} /> // this makes typescript really hard

// or worse, Input is a slot in TextField
<TextField componentsProps={{ input: { componentsProps: { data-attribute: '' } } }} />
<TextField
    {...inputProps}
    componentsProps={{
      input: { componentsProps: { readOnly: true, input: { ref: inputRef } } },
    }}
  />
  
<Select componentsProps={{ button: { aria-label: '...', id: '...' } }} />

Related issues:

Proposal

I think 99.99% of developers want to pass props/ref to the input element, not the div so why don't we mark the input as the root slot so that it still follows our convention and add a special note that the root slot does not need to be the uppermost parent of the component.

This reduces some complexity in the code as well because we don't need to define which prop should be forwarded to input.

The result will be like this:

<Input aria-label="Search" />
<Input component="textarea"  /> // typescript just works

<TextField componentsProps={{ input: { data-attribute: '' } }} />
<TextField
    {...inputProps}
    componentsProps={{
      input: { readOnly: true, ref: inputRef },
    }}
  />
  
<Select aria-label="..." id="" />

However, the sx prop is a special prop. It always goes to the uppermost of the component for styling purposes. (I tried and it gives a lot better experience).

cc @flaviendelangle What do you think?

close #33738


@siriwatknp siriwatknp added the package: joy-ui Specific to @mui/joy label Aug 3, 2022
@mui-bot
Copy link

mui-bot commented Aug 3, 2022

Details of bundle changes

@mui/joy: parsed: -0.22% 😍, gzip: -0.20% 😍

Generated by 🚫 dangerJS against ef67898

Comment on lines -214 to -225
id,
name,
onClick,
onChange,
onKeyDown,
onKeyUp,
onFocus,
onBlur,
placeholder,
readOnly,
required,
type = 'text',
Copy link
Member Author

Choose a reason for hiding this comment

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

Cleaner code 🎉

Comment on lines -278 to -291
const propsToForward = {
'aria-describedby': ariaDescribedby,
'aria-label': ariaLabel,
'aria-labelledby': ariaLabelledby,
autoComplete,
autoFocus,
id,
onKeyDown,
onKeyUp,
name,
placeholder,
readOnly,
type,
};
Copy link
Member Author

Choose a reason for hiding this comment

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

No need for all of these anymore.

Comment on lines -281 to -295
// props to forward to the button (all handlers should go through componentsProps.button)
'aria-describedby': ariaDescribedby,
'aria-label': ariaLabel,
'aria-labelledby': ariaLabelledby,
id,
name,
sx,
...other
} = props as typeof inProps & {
// need to cast types because SelectOwnProps does not have these properties
'aria-describedby'?: string;
'aria-label'?: string;
'aria-labelledby'?: string;
id?: string;
name?: string;
};
Copy link
Member Author

Choose a reason for hiding this comment

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

Look at this mess 🥲

@siriwatknp siriwatknp requested a review from a team August 4, 2022 07:50
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 4, 2022

This conformance test

export function testPropsSpread(element, getOptions) {
was created to fix the components that were not spreading props on the root 😁. The core idea was that exceptions have to be learned. If I recall correctly, we have one component in the codebase that currently skips this conformance test: Switch.

I wonder if the best solution here is to unbundle. If it's painful to target a child element, then maybe it should be its own React component? We encourage most people to use the TextField, and to look into Input when they need composition.

// or worse, Input is a slot in TextField
<TextField componentsProps={{ input: { componentsProps: { data-attribute: '' } } }} />

I think that regardless of this proposal, in some cases, it would be beneficial to flatten the slots, making child slots available to the parent.


If we move in this direction, to make sure it's compatible with making the same change in Material UI v6 (that there are no hidden constraints that we don't see). Also, I have an impression of déjà vu, I suspect we can find GitHub issues that talks about this problem in the history.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 4, 2022
@siriwatknp siriwatknp closed this Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: joy-ui Specific to @mui/joy PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Joy Select does not open when clicking the select icon
3 participants