Skip to content

[Select] Fix specificity of style overrides#25766

Merged
mnajdova merged 8 commits into
mui:nextfrom
robphoenix:patch-1
Apr 19, 2021
Merged

[Select] Fix specificity of style overrides#25766
mnajdova merged 8 commits into
mui:nextfrom
robphoenix:patch-1

Conversation

@robphoenix

Copy link
Copy Markdown
Contributor

closes #25763

@mui-pr-bot

mui-pr-bot commented Apr 14, 2021

Copy link
Copy Markdown

Details of bundle changes

Generated by 🚫 dangerJS against e793fda

@mnajdova

mnajdova commented Apr 14, 2021

Copy link
Copy Markdown
Member

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

robphoenix commented Apr 14, 2021

Copy link
Copy Markdown
Contributor Author

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

mnajdova commented Apr 14, 2021

Copy link
Copy Markdown
Member

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
Copy Markdown
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

robphoenix commented Apr 14, 2021

Copy link
Copy Markdown
Contributor Author

@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 type: bug It doesn't behave as expected. scope: select Changes related to the select. labels Apr 14, 2021
Comment thread packages/material-ui/src/Select/SelectInput.js Outdated
Comment thread packages/material-ui/src/Select/SelectInput.js Outdated
Comment thread packages/material-ui/src/Select/SelectInput.js
[`&.${selectClasses.selectMenu}`]: styles.selectMenu,
[`&.${selectClasses[styleProps.variant]}`]: styles[styleProps.variant],
},
styles.root || {},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
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

@oliviertassinari oliviertassinari Apr 15, 2021

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Probably, yes

@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
Comment thread packages/material-ui/src/Select/SelectInput.js Outdated
@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

@oliviertassinari oliviertassinari left a comment

Copy link
Copy Markdown
Member

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

scope: select Changes related to the select. type: bug It doesn't behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Select] style overrides overwritten by hardcoded values

4 participants