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] Agnostic DashboardLayout component #3831

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

apedroferreira
Copy link
Member

@apedroferreira apedroferreira commented Jul 23, 2024

Agnostic DashboardLayout so that it can be used in any app by itself, even without an AppProvider wrapping it.
Need to adjust docs, maybe adjust/update tests too.
Also remove any mentions of CSS vars theming being experimental in docs now.

@apedroferreira apedroferreira added the feature: Components Button, input, table, etc. label Jul 23, 2024
@apedroferreira apedroferreira self-assigned this Jul 23, 2024
/**
* Active palette mode in theme.
*/
paletteMode?: PaletteMode;
Copy link
Member

@Janpot Janpot Jul 23, 2024

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).

Copy link
Member Author

@apedroferreira apedroferreira Jul 23, 2024

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!)

Copy link
Member Author

@apedroferreira apedroferreira Jul 24, 2024

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.

  1. User is using DashboardLayout inside an AppProvider with any of the 3 theming methods we support (https://mui.com/toolpad/core/react-app-provider/#theming) => theme switcher already works
  2. User uses DashboardLayout without AppProvider around it => we can make things work automatically if they're using a CSS vars theme by simply using the methods from useColorScheme, 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?

Copy link
Member

@Janpot Janpot Jul 26, 2024

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

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

apedroferreira commented Jul 26, 2024

Just realized that we also need to deal with the routing if we want to make DashboardLayout work as a standalone component, but integrating that with our current system seems like a complicated problem, so not sure what the best solution is.

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:

  1. router prop in DashboardLayout just like AppProvider['router']. Ideally with this approach a specific somewhat preconfigured DashboardLayout for the Next.js app and pages routers would be made available, but then the framework-specific DashboardLayout should only be used if not using an AppProvider and that might be a bit convoluted to explain and confusing to use... Or we could make those Next.js router configurations importable from @toolpad/core and ready to be passed as props to the generic component.

  2. Maybe allow users to pass in a custom Link component to be used somehow? And as for the pathname defaulting to window.pathname might work, or it could also be passed as a prop?

  3. Considering these issues perhaps we should not make it possible to use these components standalone as the configuration doesn't seem simple/straightforward enough, as users might as well wrap the component in an AppProvider every time.

@Janpot
Copy link
Member

Janpot commented Jul 26, 2024

We can always wait with this feature until there is clear demand and user benefit.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 1, 2024
@apedroferreira apedroferreira added the on hold There is a blocker, we need to wait label Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: layout feature: Components Button, input, table, etc. on hold There is a blocker, we need to wait PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants