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

[fields] Fix usage of slotProps.textField.InputProps #8428

Merged
merged 1 commit into from Apr 3, 2023

Conversation

flaviendelangle
Copy link
Member

Part of #8322

@flaviendelangle flaviendelangle added bug 🐛 Something doesn't work component: pickers This is the name of the generic UI component, not the React module! labels Mar 29, 2023
@flaviendelangle flaviendelangle self-assigned this Mar 29, 2023
@mui-bot
Copy link

mui-bot commented Mar 29, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-8428--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 733.9 880.2 816.9 813.72 47.435
Sort 100k rows ms 661.9 1,268.2 661.9 958.36 210.371
Select 100k rows ms 269 380.2 320.5 323.96 35.766
Deselect 100k rows ms 139.8 263.7 218.9 209.1 41.284

Generated by 🚫 dangerJS against 5e839fa

@flaviendelangle flaviendelangle changed the title [fields] Fix usage of slotProps.textField.InputProps [fields] Fix usage of slotProps.textField.InputProps Mar 30, 2023
@flaviendelangle flaviendelangle marked this pull request as ready for review March 30, 2023 08:06
Comment on lines +37 to +38
textFieldProps.inputProps = { ...textFieldProps.inputProps, ...inputProps };
textFieldProps.InputProps = { ...textFieldProps.InputProps, ...InputProps };
Copy link
Member

Choose a reason for hiding this comment

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

I don't see reason to say it's a bad pattern, but it's the first time I see a mutation in React code

Copy link
Member Author

Choose a reason for hiding this comment

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

For me it's OK because we know that the object is generate on useSlotProps above, so the location is locale (it has no impact before line 28 and after line 38.

Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

I'm not sure to get why modifying the single input range and not the multi input

@flaviendelangle
Copy link
Member Author

@alexfauquette because the conflict is between props received by the field at the top level (e.g: props.InputProps) and received by the field in the textField slot (e.g: props.slotProps.textField.InputProps).
But the multi input fields don't have a top level InputProps since they spread their props to a Stack instead of a TextField.

So no need to do any manual merge, it's always the slot InputProps which wins since it's the only InputProps.

@flaviendelangle flaviendelangle merged commit 34565eb into mui:master Apr 3, 2023
18 checks passed
@flaviendelangle flaviendelangle deleted the input-props-field branch April 3, 2023 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: pickers This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants