[Select] Migrate NativeSelect to emotion by duganbrett · Pull Request #24698 · mui/material-ui

duganbrett · 2021-01-29T23:57:39Z

This PR migrates the NativeSelect and NativeSelect Input components to the new emotion format as a part of #24405.

I have followed (at least) the PR section of the contributing guide.

duganbrett · 2021-01-29T23:58:59Z

The styles from the NativeSelect file to the NativeSelectInput file to be closer to the components they were actually styling. If that's not wanted I can figure something else out.

mui-pr-bot · 2021-01-30T00:00:02Z

Details of bundle changes

@material-ui/core: parsed: +0.37% , gzip: +0.20%

Generated by 🚫 dangerJS against 5b4a4fe

packages/material-ui/src/NativeSelect/NativeSelect.js

…grate-native-select

HofmannZ · 2021-02-23T22:11:06Z

Would be great to have this in the next release 😁

oliviertassinari · 2021-04-02T19:44:03Z

@mnajdova Interested in taking this one on? It's a challenging one. I doubt the current solution is flying, it might need a maintainer to solve it.

mnajdova · 2021-04-05T21:12:20Z

@mnajdova Interested in taking this one on? It's a challenging one. I doubt the current solution is flying, it might need a maintainer to solve it.

Sure, will do

mnajdova · 2021-04-06T19:11:23Z

Ok, this is a bit tricky :)) From what I can see we should do the following:

Create NativeSelectRoot as a regular styled() component
Then from the input prop, we should use the type for the as prop, and use the input.props for spreading. This will be done instead of the React.clone. I know it's not the best but at this moment I don't have better idea.
Then inside the styled() component, we can use the classes from the NaticeSelectInput for styling, instead of the classes key.

I will try to implement these changes tomorrow on top of this PR.

Alternative would be to use the css utility, but I wouldn't like to introduce it just for this use-case as I am not sure how it would play with the overrides. If we agree that the previous proposal is bad, or if it won't work, this would be my second attempt at solving the problem.

oliviertassinari · 2021-04-06T23:29:20Z

@mnajdova The part that seemed tricky to me was that, since Select and NativeSelect aim to share most of the logic, we would need to migrate NativeSelect first, then export the component, for Select.

For the solution, instead of a classes prop, maybe we could provide a components prop. I doubt that we can remove the cloneElement because it seems that we need to add props before InputBase processes them.

mnajdova · 2021-04-07T11:02:46Z

@mnajdova The part that seemed tricky to me was that, since Select and NativeSelect aim to share most of the logic, we would need to migrate NativeSelect first, then export the component, for Select.

For the solution, instead of a classes prop, maybe we could provide a components prop. I doubt that we can remove the cloneElement because it seems that we need to add props before InputBase processes them.

I think it would be best if we convert the Select as well before merging this PR. I will branch from this branch and start converting the Select. I noticed some inconsistencies with the way of how the styles are defined, I will push those changes as part of this PR (FYI @duganbrett).

J-A-power

22

mnajdova

I did some polishing and based the #25653 on top of this one. I don't think there is anything else we are missing. Actually, it was pretty straightforward to do it, we just needed to export the styles from the NativeSelectInput (I know you had those changes initially, but I moved them to the other PR so that it would be more obvious why those are needed.)

cc @oliviertassinari in case you are interested

duganbrett · 2021-04-07T17:23:55Z

I did some polishing and based the #25653 on top of this one. I don't think there is anything else we are missing. Actually, it was pretty straightforward to do it, we just needed to export the styles from the NativeSelectInput (I know you had those changes initially, but I moved them to the other PR so that it would be more obvious why those are needed.

cc @oliviertassinari in case you are interested

Thanks for cleaning this one up and welcome back!

mnajdova · 2021-04-08T13:04:26Z

I am merging this one, we can revisit if anything else is necessary on #25653

oliviertassinari · 2021-04-08T19:25:34Z

packages/material-ui/src/NativeSelect/NativeSelect.js

  /* Styles applied to the select component `selectMenu` class. */
  selectMenu: {
-    height: 'auto', // Resets for multiple select with chips
+    height: 'auto', // Resets for multipile select with chips


Suggested change

height: 'auto', // Resets for multipile select with chips

height: 'auto', // Resets for multiple select with chips

oliviertassinari · 2021-04-08T19:27:08Z

packages/material-ui/src/NativeSelect/NativeSelectInput.js

+    },
+  }),
+  ...(styleProps.selectMenu && {
+    height: 'auto', // Resets for multpile select with chips


Suggested change

height: 'auto', // Resets for multpile select with chips

height: 'auto', // Resets for multiple select with chips

oliviertassinari · 2021-04-08T19:27:48Z

packages/material-ui/src/NativeSelect/index.d.ts

@@ -1,2 +1,4 @@
 export { default } from './NativeSelect';
 export * from './NativeSelect';
+export { default as nativeSelectClasses } from './nativeSelectClasses';


Convention with the other files

Suggested change

export { default as nativeSelectClasses } from './nativeSelectClasses';

export { default as nativeSelectClasses } from './nativeSelectClasses';

oliviertassinari · 2021-04-08T19:28:07Z

packages/material-ui/src/NativeSelect/index.js

@@ -1 +1,3 @@
 export { default } from './NativeSelect';
+export { default as nativeSelectClasses } from './nativeSelectClasses';


Convention with the other files

Suggested change

export { default as nativeSelectClasses } from './nativeSelectClasses';

export { default as nativeSelectClasses } from './nativeSelectClasses';

mnajdova · 2021-04-09T10:35:23Z

@oliviertassinari requested changes addressed in 7b09f14

[TextField] Migrate NativeSelect to emotion

429a887

duganbrett commented Jan 30, 2021

View reviewed changes

packages/material-ui/src/NativeSelect/NativeSelect.js Outdated Show resolved Hide resolved

github-actions bot mentioned this pull request Jan 30, 2021

Update snapshots for pr/24698 eps1lon/mui-scripts-incubator#1800

Closed

oliviertassinari added the scope: text field Changes related to the text field. label Jan 30, 2021

oliviertassinari mentioned this pull request Jan 30, 2021

Migrate all components to emotion #24405

Closed

duganbrett added 8 commits January 30, 2021 08:39

Merge branch 'next' of https://github.com/mui-org/material-ui into mi…

ca115b7

…grate-native-select

Fixed overrides and ignore tests.

a0bd515

Fixed formatting

3299123

Fix component name

871870e

Fix overrideResolvers and prepare for compatibility with Select

12d3570

Formattins fix

54eb6e3

rerun ci

f3a2966

rerun ci

ab9ffd5

oliviertassinari self-assigned this Feb 23, 2021

oliviertassinari added the on hold There is a blocker, we need to wait. label Mar 9, 2021

oliviertassinari removed the on hold There is a blocker, we need to wait. label Apr 2, 2021

oliviertassinari removed their assignment Apr 2, 2021

mnajdova added 2 commits April 7, 2021 13:18

misc fixes

e09f641

move styles to Select

e80316c

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Apr 7, 2021

mnajdova added 2 commits April 7, 2021 13:43

merge conflicts

f21473e

Merge branch 'next' into migrate-native-select

c2bbdc9

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Apr 7, 2021

fixes

0808efb

mnajdova mentioned this pull request Apr 7, 2021

[Select] Migrate to emotion #25653

Merged

mnajdova added the scope: select Changes related to the select. label Apr 7, 2021

mnajdova changed the title ~~[TextField] Migrate NativeSelect to emotion~~ [Select] Migrate NativeSelect to emotion Apr 7, 2021

reverted select changes

5b4a4fe

J-A-power reviewed Apr 7, 2021

View reviewed changes

mnajdova approved these changes Apr 7, 2021

View reviewed changes

mnajdova merged commit 0fc7823 into mui:next Apr 8, 2021

oliviertassinari reviewed Apr 8, 2021

View reviewed changes

	height: 'auto', // Resets for multipile select with chips
	height: 'auto', // Resets for multiple select with chips

	height: 'auto', // Resets for multpile select with chips
	height: 'auto', // Resets for multiple select with chips

	export { default as nativeSelectClasses } from './nativeSelectClasses';

	export { default as nativeSelectClasses } from './nativeSelectClasses';

		@@ -1 +1,3 @@
		export { default } from './NativeSelect';
		export { default as nativeSelectClasses } from './nativeSelectClasses';

Uh oh!

Conversation

duganbrett commented Jan 29, 2021

Uh oh!

duganbrett commented Jan 29, 2021

Uh oh!

mui-pr-bot commented Jan 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

HofmannZ commented Feb 23, 2021

Uh oh!

oliviertassinari commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mnajdova commented Apr 5, 2021

Uh oh!

mnajdova commented Apr 6, 2021

Uh oh!

oliviertassinari commented Apr 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mnajdova commented Apr 7, 2021

Uh oh!

J-A-power left a comment

Choose a reason for hiding this comment

Uh oh!

mnajdova left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duganbrett commented Apr 7, 2021

Uh oh!

mnajdova commented Apr 8, 2021

Uh oh!

oliviertassinari Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

mnajdova commented Apr 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mui-pr-bot commented Jan 30, 2021 •

edited

Loading

oliviertassinari commented Apr 2, 2021 •

edited

Loading

oliviertassinari commented Apr 6, 2021 •

edited

Loading

mnajdova left a comment •

edited

Loading