-
-
Notifications
You must be signed in to change notification settings - Fork 293
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] Agnostic DashboardLayout component #3831
base: master
Are you sure you want to change the base?
[core] Agnostic DashboardLayout component #3831
Conversation
/** | ||
* Active palette mode in theme. | ||
*/ | ||
paletteMode?: PaletteMode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe the state for paletteMode
and setPaletteMode
(we use a different naming convention for controlled props anyway) should be owned by DashboardLayout
. It's owned by the theme and if the user wants to manipulate it useColorScheme
should be used. For the classic theme, I believe we should create a useColorScheme
shim (i.e. rewrite useStandardPaletteMode
with the same signature as useColorScheme
and export from the library).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it was just a first idea and I wasn't sure about the names for these 2 props either.
We can go with that, the only disadvantage I guess is that it will only work with MUI theming (nevermind, the theme is strongly connected to the component anyway so this should be more automatic, sounds good!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking a bit more into this and not sure about the approach to take.
- User is using
DashboardLayout
inside anAppProvider
with any of the 3 theming methods we support (https://mui.com/toolpad/core/react-app-provider/#theming) => theme switcher already works - User uses
DashboardLayout
withoutAppProvider
around it => we can make things work automatically if they're using a CSS vars theme by simply using the methods fromuseColorScheme
, otherwise if they're using a standard theme it can only have one mode unless they're managing the active theme state themselves somehow, in which case we can't support it automatically?
About the shim, what scenario are we trying to solve with it? For internal components we have this context https://github.com/mui/mui-toolpad/blob/ad9bddb719a81cd113b73e82d304ae5ead1a0e8c/packages/toolpad-core/src/DashboardLayout/DashboardLayout.tsx#L81
Should I make an abstraction around the context as a hook that users can import called useColorScheme
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we could make it a dumb controlled property. But name it colorScheme
, onColorSchemeChange
to match naming of the cssvar theme, and also we always use onXyzChange
for controlled prop xyz
, not setXyz
Just realized that we also need to deal with the routing if we want to make There are a few possible approaches but maybe none of them sounds ideal, plus don't want to go too far in any of them without first aligning on a solution:
|
We can always wait with this feature until there is clear demand and user benefit. |
Agnostic
DashboardLayout
so that it can be used in any app by itself, even without anAppProvider
wrapping it.Need to adjust docs, maybe adjust/update tests too.
Also remove any mentions of CSS vars theming being experimental in docs now.