Skip to content

[Select][NativeSelect] Polish CSS classes#26186

Merged
m4theushw merged 9 commits intomui:nextfrom
m4theushw:polish-select-classes
May 13, 2021
Merged

[Select][NativeSelect] Polish CSS classes#26186
m4theushw merged 9 commits intomui:nextfrom
m4theushw:polish-select-classes

Conversation

@m4theushw
Copy link
Collaborator

@m4theushw m4theushw commented May 7, 2021 •
edited by oliviertassinari
Loading

Breaking changes

  • [Select] The selectMenu slot was merged into select because selectMenu slot was redundant. The root slot is no longer applied to the select, but to the root.

    -<Select classes={{ root: 'class1', select: 'class2', selectMenu: 'class3' }} />
+<Select classes={{ root: 'class1', select: 'class2 class3' }} />

  • [NativeSelect] The selectMenu slot was merged into select because selectMenu slot was redundant. The root slot is no longer applied to the select, but to the root.

    -<NativeSelect classes={{ root: 'class1', select: 'class2', selectMenu: 'class3' }} />
+<NativeSelect classes={{ select: 'class1 class2 class3' }} />

  • [TablePagination] The classes.input key was changed to be applied on another element. Use classes.select instead.

    <TablePagination
- classes={{ input: 'foo' }}
+ classes={{ select: 'foo' }}
/>

Part of #20012
Closes #11646
Closes #25313

I couldn't understand very well the discussion that lead to this change. I found #25766 (comment) about removing root and select. But the TODO in the code was to remove selectMenu. If it was not the specificity bug raised in #25763, we would only need to keep root.

@mui-pr-bot
Copy link

mui-pr-bot commented May 7, 2021 •
edited
Loading

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against edd965d

mnajdova
mnajdova reviewed May 7, 2021
View reviewed changes
@oliviertassinari oliviertassinari added the scope: select Changes related to the select. label May 7, 2021
@m4theushw m4theushw mentioned this pull request May 7, 2021
Closed
@github-actions github-actions bot mentioned this pull request May 8, 2021
Closed
oliviertassinari
oliviertassinari reviewed May 11, 2021
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Almost good to go from my end

packages/material-ui/src/Select/SelectInput.js
overridesResolver: (props, styles) => {
const { styleProps } = props;
return {
[`&.${selectClasses.select}`]: {
Copy link
Member

@oliviertassinari oliviertassinari May 11, 2021

Choose a reason for hiding this comment

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

We could add a comment on why we bump the specificity, from what I remember, it's important to guarantee that the customization is applied. The base style comes from the InputBase-input which we have no injection order guarantee with.

Ah yes, we say "// Win specificity over the input base" below

Copy link
Member

@oliviertassinari oliviertassinari May 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We need to keep the bump:

Suggested change
[`&.${selectClasses.select}`]: {
// Win specificity over the input base
'&': {

Copy link
Member

@oliviertassinari oliviertassinari May 12, 2021

Choose a reason for hiding this comment

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

@m4theushw what do you think about this comment?

Copy link
Collaborator Author

@m4theushw m4theushw May 12, 2021

Choose a reason for hiding this comment

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

Updated. Missed this one because there's already a comment below.

@oliviertassinari oliviertassinari added the breaking change Introduces changes that are not backward compatible. label May 11, 2021
oliviertassinari
oliviertassinari reviewed May 11, 2021
View reviewed changes
m4theushw
m4theushw commented May 12, 2021
View reviewed changes
@m4theushw m4theushw changed the title [Select] Polish CSS classes [Select][NativeSelect] Polish CSS classes May 12, 2021
@m4theushw m4theushw merged commit e526be5 into mui:next May 13, 2021
@m4theushw m4theushw deleted the polish-select-classes branch May 13, 2021 18:53
@joserodolfofreitas joserodolfofreitas mentioned this pull request Aug 4, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mnajdova mnajdova mnajdova left review comments

@oliviertassinari oliviertassinari oliviertassinari approved these changes

Assignees

No one assigned

Labels

breaking change Introduces changes that are not backward compatible. scope: select Changes related to the select.

Projects

None yet

Milestone

No milestone

4 participants

@m4theushw @mui-pr-bot @oliviertassinari @mnajdova