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

[base-ui] Fix mergeSlotProps className join order #39616

Merged

Conversation

@mj12albert mj12albert added bug 🐛 Something doesn't work package: base-ui Specific to @mui/base labels Oct 26, 2023
@mui-bot
Copy link

mui-bot commented Oct 26, 2023

Netlify deploy preview

https://deploy-preview-39616--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 9b6f48e

@mj12albert
Copy link
Member Author

After #39032 (comment) I have been leaving the className prop in externalForwardedProps (in FormControl, FilledInput) @DiegoAndai

It turns outs if className is explicitly passed to the className parameter of mergeSlotProps (AND if getSlotProps is not passed), it hides the problem that the className merge order is wrong, since className overrides externalSlotProps?.className

Luckily InputLabel contained this test and it crossed my path 😅 otherwise the issue could have stayed hidden for a lot longer

@mj12albert mj12albert marked this pull request as ready for review October 27, 2023 07:19
@DiegoAndai
Copy link
Member

I agree with these changes 👍🏼 there should be consistency.

I won't approve yet because I don't have the context about which order is correct, so I would wait for either @mnajdova or @michaldudak to review.

Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

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

Nice catch and fix! Implementing Material UI components and testing them with Base UI primitives is the best thing that could happen for this library.

@mj12albert mj12albert merged commit 8ab0e65 into mui:master Oct 31, 2023
19 checks passed
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Oct 31, 2023
@mj12albert mj12albert deleted the base/merge-slot-props-classes-join-order branch November 2, 2023 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work package: base-ui Specific to @mui/base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[base-ui] Inconsistent merge order of classes in mergeSlotProps
4 participants