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

[core] Add theme switcher to dashboard layout #3674

Merged

Conversation

apedroferreira
Copy link
Member

@apedroferreira apedroferreira commented Jun 12, 2024

Closes #3586

Add theme switcher to DashboardLayout component header.

Screen.Recording.2024-07-03.at.08.38.16.mov

As decided below, the theme can be set as Theme | CssVarsTheme | { light: Theme, dark: Theme }; , and the switcher only shows if using both light and dark themes both for the CSS variables or the standard option.

Also moved all React contexts to shared/context while trying to deal with some circular dependency issues + trying not to export them with the package, but this might not have been necessary in the end. Let me know if you think we should organize that differently.

Documented theming + the switcher in the AppProvider and DashboardLayout docs, and tried to improve the documentation for both these components in general as it had been discussed:

Component Pages:

API Pages:

@apedroferreira apedroferreira added the feature: Components Button, input, table, etc. label Jun 12, 2024
@apedroferreira apedroferreira self-assigned this Jun 12, 2024
@apedroferreira
Copy link
Member Author

apedroferreira commented Jun 13, 2024

There is an issue during SSR when using the dark mode in Next.js where the server can't read which mode it's supposed to use, so the app renders in light mode until the client-side sets the correct value for the dark mode.

From what I've tried/researched there's 2 possible solutions:

  1. Use cookies to store the theme mode - will eventually work but it's complicated as it has to be done in different ways for the app router and the pages router. The app router needs to use a server component and the pages router an alternative solution - these can be difficult and a bit messy to integrate with the library.
  2. Leave it as is and probably Pigment.css will provide a more elegant solution for this?

@Janpot
Copy link
Member

Janpot commented Jun 13, 2024

  1. We should be able to leverage css variables like we do on the docs homepage.

What if we do?

interface AppProviderProps {
  theme?: Theme | CssVarsTheme | { light: Theme, dark: Theme }; 
  // ...
}

We show the switcher when

  • { light: Theme, dark: Theme }
  • CssVarsTheme AND colorSchemes.light AND colorSchemes.dark are set
  • default theme = CssVarsTheme with both color schemes

Otherwise no theme switcher

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 19, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 25, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 28, 2024
@prakhargupta1 prakhargupta1 linked an issue Jul 2, 2024 that may be closed by this pull request
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 2, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 12, 2024
pathname: PropTypes.string.isRequired,
};

const customTheme = createTheme({
Copy link
Member

Choose a reason for hiding this comment

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

Follow-up PR: Would it make sense to make this a CSS vars theme and provide custom dark and light color schemes? We do want to promote this method over the legacy theme.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could do it, it's actually a small change so might include it in this PR.

@@ -8,22 +8,35 @@ components: AppProvider, DashboardLayout

<p class="description">The dashboard layout component provides a customizable out-of-the-box layout for a typical dashboard page.</p>

The `DashboardLayout` component is a quick, easy way to provide a standard full-screen layout with a header and sidebar to any dashboard page, as well as ready-to-use and easy to customize navigation and branding.

Many features of this component are configurable through the [AppProvider](https://mui.com/toolpad/core/react-app-provider/) component that should wrap it.
Copy link
Member

Choose a reason for hiding this comment

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

To follow-up outside of this PR:
Would it make sense to provide all the available functionality that is read from the app provider as props as well? That way the suer has the choice to use with app provider and stand alone. If we let the props fall-back to the app provider contexts we have a way to offer following functionalities:

  • use with app provider
  • use standalone with the props
  • use with app provider but overwrite specific context

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I was planning to do that next and then edit the docs accordingly, we can prioritize the props that are passed otherwise default to the app provider.

title: 'MUI',
}}
router={router}
theme={{ light: baseLightTheme, dark: baseDarkTheme }}
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this property here? If we need it, I'd expect it to set the primary color to one that matches the logo.

I also don't think we should promote anything other than css vars themes. The only place where we mention or use legacy theme should be where we document specifically them.

Copy link
Member Author

@apedroferreira apedroferreira Jul 16, 2024

Choose a reason for hiding this comment

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

You mean the theme property?
There's actually some conflict when using the CSS vars theme in the docs examples which is why I'm using theme={{ light: baseLightTheme, dark: baseDarkTheme }}... Maybe the variable names for the iframes can be given a different name or something and that will work, I'll check out what else we can do there.

Copy link
Member

Choose a reason for hiding this comment

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

You mean the theme property?

Yes, can't this demo just rely on the default theme?

Copy link
Member Author

@apedroferreira apedroferreira Jul 19, 2024

Choose a reason for hiding this comment

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

It's using the default CSS variables theme now including in docs examples, but had to add a window prop to the AppProvider to be able to use the docs demo window instead of the whole page's.
Let me know if you think any alternative would be better and we can change it, even if in a separate PR.

// preview-start
<AppProvider
navigation={NAVIGATION}
branding={{
Copy link
Member

@Janpot Janpot Jul 16, 2024

Choose a reason for hiding this comment

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

For a follow-up conversation:

Would it make sense to instead move these into the theme as custom tokens?


{{"demo": "AppProviderTheme.js", "height": 500, "iframe": true}}

### Predefined themes
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure about this. Isn't the predefined theme just the default MUI theme? I'm not sure providing different themes should be in the scope of @toolpad/core. Or is there a specific reason?

In any case, I think we need to exclusively promote css vars themes and leave the legacy functionality for when people bring their own existing theme.

Copy link
Member Author

@apedroferreira apedroferreira Jul 16, 2024

Choose a reason for hiding this comment

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

Doesn't necessarily need to be in the scope, I just wanted to possibly provide some ready-to-use themes that will look good. And that default theme just has a few color changes to look a bit better, but I guess the default MUI theme should look decent enough.

To keep things simple let's keep themes outside the library then, and will recheck how the default MUI theme looks - if it looks good we can keep it as the default.

@apedroferreira
Copy link
Member Author

apedroferreira commented Jul 18, 2024

TODO after this PR:

Bugs:

  • When using CSS vars theme in docs the docs initial theme is being changed if theme is changed in demos
  • AppBar styles throwing console error in Material UI v6 when loading page with dark theme

Docs:

  • Better looking light theme in app provider theme demo

Features:

Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

Let's merge the current state and follow up on future PRs

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 19, 2024
@apedroferreira
Copy link
Member Author

Merging, thanks!
Some bugs left to fix which are mentioned above.

@apedroferreira apedroferreira changed the title Add theme switcher to dashboard layout (Toolpad Core) [core] Add theme switcher to dashboard layout Jul 19, 2024
@apedroferreira apedroferreira merged commit 98b87ad into mui:master Jul 19, 2024
13 of 14 checks passed
@apedroferreira apedroferreira deleted the add-dashboard-layout-theme-switcher branch July 19, 2024 12:40
@@ -279,9 +194,9 @@ export default function generateProject(
'react-dom': '^18',
next: '^14',
'@toolpad/core': 'latest',
'@mui/material': '^5',
'@mui/material': '^6',
Copy link
Member

Choose a reason for hiding this comment

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

@apedroferreira I believe this will broken, this version doesn't exist on npm.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok thanks for finding it, I'm working on bug fixes so will make sure the CLI and generated project work along with them.

Copy link
Member

@Janpot Janpot Jul 19, 2024

Choose a reason for hiding this comment

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

For now I changed these ones to 'next' in #3807

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, didn't know you were releasing or might have checked more carefully. Thanks!

@apedroferreira apedroferreira mentioned this pull request Jul 19, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: Components Button, input, table, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DashboardLayout] Add theme palette mode switcher
3 participants