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

[Select] Fix specificity of style overrides #25766

Merged
merged 8 commits into from
Apr 19, 2021
Merged

Conversation

robphoenix
Copy link
Contributor

closes #25763

@mui-pr-bot
Copy link

mui-pr-bot commented Apr 14, 2021

Details of bundle changes

Generated by 🚫 dangerJS against e793fda

@mnajdova
Copy link
Member

mnajdova commented Apr 14, 2021

filled and outlined are already supported by styles[variant]. I noticed another problem here - ovrerides for the slots icon and nativeInput can't work currently, as they are not children of the SelectRoot. This can be enabled in the future via the components prop. So for now I'd suggest we do this in addition:

diff --git a/packages/material-ui/src/Select/SelectInput.js b/packages/material-ui/src/Select/SelectInput.js
index 46d3f20618..25ede7c405 100644
--- a/packages/material-ui/src/Select/SelectInput.js
+++ b/packages/material-ui/src/Select/SelectInput.js
@@ -15,17 +15,14 @@ import useForkRef from '../utils/useForkRef';
 import useControlled from '../utils/useControlled';
 import selectClasses, { getSelectUtilitiyClasses } from './selectClasses';

+// TODO: enable overrides for the icon and nativeInput slots via the components prop
 export const overridesResolver = (props, styles) => {
   const { styleProps } = props;
   return deepmerge(
     {
       ...styles.select,
       ...styles[styleProps.variant],
-      [`& .${selectClasses.icon}`]: {
-        ...styles.icon,
-        ...(styleProps.variant && styles[`icon${capitalize(styleProps.variant)}`]),
-        ...(styleProps.open && styles.iconOpen),
-      },
+      [`&.${selectClasses.selectMenu}`]: styles.selectMenu,
     },
     styles.root || {},
   );

@robphoenix
Copy link
Contributor Author

robphoenix commented Apr 14, 2021

So for now I'd suggest we do this in addition

@mnajdova The outlined style overrides appear to be being overwritten by the hardcoded values? In this case, the height/min-height CodeSandbox

image

robphoenix and others added 2 commits April 14, 2021 12:06
@mnajdova overrides for the slots icon and nativeInput can't work currently, as they are not children of the SelectRoot.
This can be enabled in the future via the components prop.
@mnajdova
Copy link
Member

mnajdova commented Apr 14, 2021

Ok, I think the root problem is the bumbed sepcificy for the selectMenu in the SelectRoot. I've tried 24b9dd4 let's see if it will cause any other problems, if this work, we can add tests to ensure we don't have regressions the future

I remembered why we needed it, it was because of the nature of how emotion rights it's styles. We need to bump the specificity for the overrides 0ae47d9 @robphoenix can you check if this solves all the issues you could spot?

@mnajdova
Copy link
Member

I noticed another inconsistency with this component. It spreads the props from theme's components' default props to the root input component, but the overrides are applied to the select root component. Is the spreading of the default prop correct? Do we expect them to be on the input component? If yes, we need to write manual test for the theme style & variant overrides. cc @oliviertassinari

@robphoenix
Copy link
Contributor Author

robphoenix commented Apr 14, 2021

@mnajdova this solves my original issue CodeSandbox

image

Thankyou 😃

@robphoenix robphoenix changed the title [Select] Add overrides for selectMenu [Select] Change specificity of style overrides Apr 14, 2021
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: select This is the name of the generic UI component, not the React module! labels Apr 14, 2021
packages/material-ui/src/Select/SelectInput.js Outdated Show resolved Hide resolved
...(styleProps.open && styles.iconOpen),
},
[`&.${selectClasses.selectMenu}`]: styles.selectMenu,
[`&.${selectClasses[styleProps.variant]}`]: styles[styleProps.variant],
},
styles.root || {},
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Referencing #11646 where we plan to simply this

Copy link
Member

@oliviertassinari oliviertassinari Apr 15, 2021

Choose a reason for hiding this comment

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

And #25313, which suggest we have no root on this file so: root, select, selectMenu.

Copy link
Member

Choose a reason for hiding this comment

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

Should we do this in a separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

Probably, yes

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 16, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 16, 2021
@oliviertassinari oliviertassinari changed the title [Select] Change specificity of style overrides [Select] Fix specificity of style overrides Apr 16, 2021
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

I have push an extra commit, to try to push the approach one step ahead

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: select 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.

[Select] style overrides overwritten by hardcoded values
4 participants