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][FormControl] Make outlined variant the default one #24895

Merged
merged 13 commits into from
Apr 16, 2021

Conversation

petyosi
Copy link
Contributor

@petyosi petyosi commented Feb 13, 2021

Breaking changes


Change the default variant from standard to outlined. Rationale in Material Studies: Text Fields. Done for TextField in #23503.

One of #20012

Checklist:

  • Change the default variant of the Select component
  • Change the default variant of the NativeSelect component better be done in a separate PR, not a small thing, IMO
  • Update the Select docs
  • Review the variant usage of the FormControl and see if it is safe to change it
  • Entry in the migration guide
  • Change docs/pages/api-docs/select.json (or maybe it is generated?)
  • In the docs, remove all explicit variant="outlined" props
  • Where necessary, add variant="standard" to selects who had no variant before
  • Codemod like this + test
  • Review tests
  • Review <TextField select>

@mbrookes - please let me know if I am missing something from the above. FWIW, the biggest inconvenience of the outlined Select is that the label prop value should match the InputLabel text.

@mui-pr-bot
Copy link

mui-pr-bot commented Feb 13, 2021

Details of bundle changes

Generated by 🚫 dangerJS against 3cdb209

@oliviertassinari oliviertassinari added breaking change component: select This is the name of the generic UI component, not the React module! labels Feb 13, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 13, 2021

FWIW, the biggest inconvenience of the outlined Select is that the label prop value should match the InputLabel text.

True. I think that most developers use the composed version (<TextField select>). It's handled for them. We have #21782 to better solve this problem.

@mbrookes
Copy link
Member

Change docs/pages/api-docs/select.json (or maybe it is generated?)

yarn docs:api will take care of it

@mbrookes
Copy link
Member

Codemod like this + test

Or modify it to support both use-cases

@petyosi petyosi force-pushed the default-outlined-select branch 2 times, most recently from 42b54d9 to 349a9ed Compare February 15, 2021 06:38
@petyosi
Copy link
Contributor Author

petyosi commented Feb 15, 2021

@mbrookes @oliviertassinari I need your advice on how to scope this changeset. This change seems to be triggering multiple related ones. Here's what I am seeing so far:

The Select component obtains its default variant from the parent FormControl, which is "standard" atm. Based on the examples, I assume that a significant amount of users wrap Select in FormControl.

With that in mind, I see two possibilities:

A) Change the default variant of the Select, and leave the FormControl change for a follow-up PR. Smaller and safer, but to put it simply - won't do much, apart from the users who use it without a FormControl. This approach also raises the question of what should be done for the docs page - should all FormControl wrappers get variant="outlined"? Or should we count that the follow-up PR will take care of that?

B) Change the default variant of FormControl to "outlined". This is what I have been exploring so far, but it ripples through several areas - see the argos build failure. The variant change affects the default look of FormHelperText and InputLabel. It also subtly breaks such compositions:

<FormControl ...>
... <Input />
</FormControl>

From what I see, for the above to look correct, Input should be replaced with OutlinedInput.

Maybe there's C) another approach?

FWIW, I am very supportive of outphasing the "standard" variant of selects and inputs. It definitely feels like a usability mistake in hindsight.

@oliviertassinari
Copy link
Member

B) Sounds like the best way to go. It does involve diving into multiple parts of the codebase.

@oliviertassinari
Copy link
Member

@petyosi How is the effort going? Did you face a blocker?

@petyosi
Copy link
Contributor Author

petyosi commented Mar 7, 2021

Hey, sorry for the delay, Working through the screenshot differences, I will push an update for review this week.

@petyosi petyosi marked this pull request as ready for review March 9, 2021 05:20
@petyosi
Copy link
Contributor Author

petyosi commented Mar 9, 2021

@mbrookes @oliviertassinari Please take a look, thanks!

@petyosi petyosi changed the title [Select] Make outlined variant the default one [Select,FormControl] Make outlined variant the default one Mar 9, 2021
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 18, 2021
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

The changes look great. One thing I am not sure about is, whether the standard variant should still be named standard as it is not a default value. I feel like maybe we should rename to something like underlined variant (or anything else that is not standard, as it not being default value means it is not standard :))

export default function TextFieldComponent(props) {
return (
<div>
<TextField {...props} variant="standard" />
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<TextField {...props} variant="standard" />
<TextField variant="standard" {...props} />

Shouldn't this be the result? Otherwise, we are always overriding any variant being part of props. The same would be applied to the other components if this is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your comment seems correct to me - the codemod was originally introduced by @mbrookes and I basically multiplied it.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, ok let's consider fixing it in a separate PR.

@oliviertassinari
Copy link
Member

whether the standard variant should still be named standard as it is not a default value.

@mnajdova If I recall correctly, the name originates from when we introduced the filled, and outlined variant. It likely has this meaning:

a pattern or model that is generally accepted:

https://dictionary.cambridge.org/dictionary/english/standard

It doesn't age well if it's not the default, agree. Should it be enough to make a breaking change? I guess we have to weigh the cost. cc @mbrookes. In any case, I think that it's outside the scope of this PR and should concern a potential follow-up. Personally, I think that it would cost a lot for some value, maybe not enough.

@mnajdova
Copy link
Member

I think that it's outside the scope of this PR and should concern a potential follow-up. Personally, I think that it would cost a lot for some value, maybe not enough.

Agree, it is definitely out of the scope of this PR, just thought to mention it as we are making the changes now. So far developers were never typing variant="standard", as it was the default value. Now that it is not a default value, they would need to add it, so my thought was instead of adding variant="standard", they could add variant="underlined". But on the other hand, I get that it may not be work the effort, but let's at least consider it before making the final decision.

Let me update the branch with the latest changes, so we can take a look on the changes & visual regressions better.

docs/src/pages/components/checkboxes/CheckboxesGroup.tsx Outdated Show resolved Hide resolved
docs/src/pages/components/radio-buttons/ErrorRadios.tsx Outdated Show resolved Hide resolved
@@ -91,6 +91,7 @@ const Select = React.forwardRef(function Select(props, ref) {
: classes,
...(input ? input.props.inputProps : {}),
},
...(multiple && variant === 'outlined' ? { notched: true } : {}),
Copy link
Member

Choose a reason for hiding this comment

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

Is there more context on this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

without the notched prop, the label was strike through the fieldset border. My guess is that this has always been the case, but the combination (outlined + multiple) was never visible.

Copy link
Member

Choose a reason for hiding this comment

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

The visual regression test caught this:

Screenshot 2021-04-14 at 21 29 44

Copy link
Member

Choose a reason for hiding this comment

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

This one might be related as well:

Screenshot 2021-04-14 at 21 30 32

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch on the first one. I was trying to fix the native + multiple + outlined combo. Pushed a fix.

The second one is not related. It is "right" I believe, although it does not look very well. The example itself is quite extreme, as the width is very small (I guess in order to test the wrapping of the chips).

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 14, 2021
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 15, 2021
@oliviertassinari oliviertassinari changed the title [Select,FormControl] Make outlined variant the default one [Select][FormControl] Make outlined variant the default one Apr 15, 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 15, 2021
@oliviertassinari oliviertassinari merged commit 5a8645d into mui:next Apr 16, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 16, 2021

One more BC done 🙏 @petyosi thanks

@petyosi petyosi deleted the default-outlined-select branch April 17, 2021 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change 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.

5 participants