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

[AppBar] Fix background color on dark mode #26545

Merged
merged 13 commits into from
Jun 3, 2021

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jun 1, 2021

Breaking changes

  • The color prop has no longer any effect in dark mode. The app bar uses the background color required by the elevation to follow the Material Design guidelines. Use enableColorOnDark to restore the behavior of v4.

    <AppBar enableColorOnDark />

closes #18308

Preview https://deploy-preview-26545--material-ui.netlify.app/components/app-bar/

The case of AppBar is special because the specified color should be enabled only in light mode. I propose the fix by adding another flag (naming can be discussed) to neglect the color prop in dark mode. This approach works great because AppBar inherit Paper which support elevation already. The flag is true by default (consider breaking change) due to the material design spec

Even bottom appbar also use elevation https://material.io/design/color/dark-theme.html#custom-application

Default Behavior

<AppBar color="primary" /> // color has no effect in dark mode

Enable for specific component (opt out via prop)

<AppBar color="primary" enableColorOnDark />

Enable globally (opt out via theme)

createTheme({
  components: {
    MuiAppBar: {
      defaultProps: {
        enableColorOnDark: true,
      }
    }
  }
})

@mui-pr-bot
Copy link

mui-pr-bot commented Jun 1, 2021

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against 103b5cb

@siriwatknp siriwatknp changed the title [AppBar] add skipColorOnDark prop to disable color in dark mode [AppBar] Automatically skip color on dark mode Jun 1, 2021
@siriwatknp siriwatknp changed the title [AppBar] Automatically skip color on dark mode [AppBar] Automatically neglect color prop on dark mode Jun 1, 2021
@siriwatknp siriwatknp changed the title [AppBar] Automatically neglect color prop on dark mode [AppBar] Automatically neglect color on dark mode Jun 1, 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.

A quick review. Could we also remove the custom CSS we apply for the app bar of the documentation?

packages/material-ui/src/AppBar/AppBar.js Outdated Show resolved Hide resolved
packages/material-ui/src/AppBar/AppBar.d.ts Outdated Show resolved Hide resolved
@siriwatknp siriwatknp changed the title [AppBar] Automatically neglect color on dark mode [AppBar] neglect color on dark mode by default Jun 2, 2021
@siriwatknp siriwatknp marked this pull request as ready for review June 2, 2021 05:47
@oliviertassinari oliviertassinari added the component: app bar This is the name of the generic UI component, not the React module! label Jun 2, 2021
packages/material-ui/src/AppBar/AppBar.js Outdated Show resolved Hide resolved
docs/src/pages/guides/migration-v4/migration-v4.md Outdated Show resolved Hide resolved
docs/src/pages/guides/migration-v4/migration-v4.md Outdated Show resolved Hide resolved
docs/src/modules/components/AppFrame.js Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari changed the title [AppBar] neglect color on dark mode by default [AppBar] Fix background color on dark mode Jun 2, 2021
@oliviertassinari oliviertassinari added breaking change bug 🐛 Something doesn't work design: material This is about Material Design, please involve a visual or UX designer in the process labels Jun 2, 2021
siriwatknp and others added 4 commits June 2, 2021 20:15
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@@ -136,3 +136,27 @@ function HideOnScroll(props) {
);
}
```

## Enable Color on Dark
Copy link
Member

@oliviertassinari oliviertassinari Jun 3, 2021

Choose a reason for hiding this comment

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

Our heading capitalization convention:

Suggested change
## Enable Color on Dark
## Enable color on dark

Copy link
Member

Choose a reason for hiding this comment

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

We might want to explain what "on dark" mean

Suggested change
## Enable Color on Dark
## Enable color on dark mode


## Enable Color on Dark

From v5 onward, `color` prop has no effect on dark mode according to [material design spec](https://material.io/design/color/dark-theme.html). You can opt out by passing `enableColorOnDark` prop.
Copy link
Member

Choose a reason for hiding this comment

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

Prefer breaking lines between sentences, makes reviewing easier:

Suggested change
From v5 onward, `color` prop has no effect on dark mode according to [material design spec](https://material.io/design/color/dark-theme.html). You can opt out by passing `enableColorOnDark` prop.
From v5 onward, `color` prop has no effect on dark mode according to [material design spec](https://material.io/design/color/dark-theme.html).
You can opt out by passing `enableColorOnDark` prop.

Copy link
Member

Choose a reason for hiding this comment

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

We usually don't mention previous versions:

Suggested change
From v5 onward, `color` prop has no effect on dark mode according to [material design spec](https://material.io/design/color/dark-theme.html). You can opt out by passing `enableColorOnDark` prop.
The `color` prop has no effect on dark mode according to [material design spec](https://material.io/design/color/dark-theme.html).
You can opt out by passing `enableColorOnDark` prop.

Copy link
Member

Choose a reason for hiding this comment

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

@mbrookes Are you OK to settle on this terminology?

Suggested change
From v5 onward, `color` prop has no effect on dark mode according to [material design spec](https://material.io/design/color/dark-theme.html). You can opt out by passing `enableColorOnDark` prop.
The `color` prop has no effect on dark mode according to [Material Design guidelines](https://material.io/design/color/dark-theme.html).
You can opt out by passing `enableColorOnDark` prop.

Copy link
Member

Choose a reason for hiding this comment

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

@mbrookes Are you OK to settle on this terminology?

No, but it seems that it was merged before you asked 🙂.

A gentle reminder to @mui-org/maintainers to tag me on any PRs with new or significantly changed language or terminology.

Copy link
Member

Choose a reason for hiding this comment

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

@mbrookes You have another shoot at it in #26628.

@oliviertassinari
Copy link
Member

I have updated the PR description to include the breaking changes. We will use it for the changelog of the next version.

@siriwatknp siriwatknp deleted the appbar-dark-mode branch June 3, 2021 09:31
@JakeRuth
Copy link

Hi @siriwatknp
I am an engineer using this lib for a project, it's seriously a life saver really appreciate it immensely thank you all so much!

I started using material ui at "@material-ui/core": "^5.0.0-alpha.28", at that point the app bar was always the primary theme color regardless of light/dark mode.
When I updated to "@material-ui/core": "^5.0.0-alpha.36" that changed. I read/see above this was clearly intention/I understand why the change was made.

However, it did take me like 15/20 minutes of confusion with the old color not easily coming back from setting the color prop, I thought I was going crazy. But then I did some deep digging and found this.

One thing that could be helpful is if the sites documentation was updated to include the enableColorOnDark color. I'm kinda game to make a commit if someone can point me in the direction?

Again, seriously killer lib, and I'm unblocked/fixed this relatively easily.

Best,
Jake

@oliviertassinari
Copy link
Member

@JakeRuth Use the documentation of the version you are using 😁 https://next.material-ui.com/api/app-bar/

@JakeRuth
Copy link

@JakeRuth Use the documentation of the version you are using 😁 https://next.material-ui.com/api/app-bar/

I have embarrassingly using the wrong version of the libs my entire time using this lib!
I am very happy to know this, lol. Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change bug 🐛 Something doesn't work component: app bar This is the name of the generic UI component, not the React module! design: material This is about Material Design, please involve a visual or UX designer in the process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[AppBar] Make dark by default for dark themes
6 participants