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

[material-ui] Remove enableColorScheme prop from CssBaseline #42695

Closed
o-alexandrov opened this issue Jun 20, 2024 · 8 comments
Closed

[material-ui] Remove enableColorScheme prop from CssBaseline #42695

o-alexandrov opened this issue Jun 20, 2024 · 8 comments
Assignees
Labels
component: CssBaseline The React component RFC Request For Comments

Comments

@o-alexandrov
Copy link
Contributor

o-alexandrov commented Jun 20, 2024

What's the problem?

There is an unused prop enableColorScheme in CssBaseline which gives an option to choose something that does not make any sense.

From #42640 (comment)

What’s the need for enableColorScheme: false?
In other words, why would anyone want the browser to show light color scheme for a dark theme?

The existence of enableColorScheme led to the introduction of an extra hanging span HTML element.

What are the requirements?

Remove enableColorScheme prop

What are our options?

In Material UI v7+, you might consider to remove the enableColorScheme prop.

Search keywords: enableColorScheme, CssBaseline

@o-alexandrov o-alexandrov added RFC Request For Comments status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jun 20, 2024
@zannager zannager added the component: CssBaseline The React component label Jun 20, 2024
@oliviertassinari
Copy link
Member

The prop was added to avoid breaking changes, as far as I remember.

This issue looks like point 4 of #38507 but where we also remove the prop.

@o-alexandrov
Copy link
Contributor Author

o-alexandrov commented Jun 20, 2024

@oliviertassinari thank you for linking the issue and specifying the point 4. Would you like to reword the 4th point there, so it says removal, instead of setting the value to true by default?

  • if yes, then I’d happily suggest to close this issue as this one would become a duplicate

@siriwatknp
Copy link
Member

I think we can remove it before the stable release of v6. It's a breaking change but I think CssBaseline is used once in a project.

@siriwatknp siriwatknp added v6.x and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jun 26, 2024
@siriwatknp siriwatknp added this to the Material UI: v6 milestone Jun 26, 2024
@siriwatknp siriwatknp added the good first issue Great for first contributions. Enable to learn the contribution process. label Jul 8, 2024
@siriwatknp siriwatknp changed the title [RFC] Remove enableColorScheme prop from CssBaseline [material-ui] Remove enableColorScheme prop from CssBaseline Jul 8, 2024
@headironc
Copy link

I'll try completing this.

@chrisbellstardust
Copy link

There doesnt seem to be much documentation on actually removing props -- after starting this change I realized this isn't really just a code change and more may be needed.

  • Should the existence of the prop from the css baseline component on the docs be removed?

This issue might be "fixed" faster if we just default the enableColorScheme to true and deprecate the prop?

@siriwatknp
Copy link
Member

@DiegoAndai I have 3 options here:

  1. set enableColorScheme: true by default and update the migration docs
  2. replace the enableColorScheme with disableColorScheme (meaning the color-scheme is enabled by default too), then update the migration docs
  3. postpone this to v7

The difference between 1 and 2 is that the latter follow our convention for using boolean flags.

Both are considered breaking changes. Should we do it or should we postpone to v7?

@DiegoAndai
Copy link
Member

@siriwatknp I think we should move this request to be included/considered in #38507, and close this one as duplicate as @o-alexandrov suggested here.

This would effectively postpone this to v7 so we can have enough time to rework CssBaseline properly.

@DiegoAndai
Copy link
Member

I've moved this request to point 4 in #38507. Closing as duplicate.

@DiegoAndai DiegoAndai closed this as not planned Won't fix, can't repro, duplicate, stale Aug 8, 2024
@DiegoAndai DiegoAndai removed this from the Material UI: v6 milestone Aug 8, 2024
@DiegoAndai DiegoAndai removed v6.x good first issue Great for first contributions. Enable to learn the contribution process. labels Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: CssBaseline The React component RFC Request For Comments
Projects
None yet
Development

No branches or pull requests

8 participants