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

[docs] Improve Toolpad Core docs #43796

Merged
merged 20 commits into from
Oct 15, 2024

Conversation

@bharatkashyap bharatkashyap added the docs Improvements or additions to the documentation label Sep 17, 2024
@mui-bot
Copy link

mui-bot commented Sep 17, 2024

Netlify deploy preview

@material-ui/core: parsed: +Infinity% , gzip: +Infinity%
@mui/joy: parsed: +Infinity% , gzip: +Infinity%
@material-ui/lab: parsed: +Infinity% , gzip: +Infinity%
@mui/joy/Autocomplete: parsed: +Infinity% , gzip: +Infinity%
Autocomplete: parsed: +Infinity% , gzip: +Infinity%
TextField: parsed: +Infinity% , gzip: +Infinity%
@material-ui/unstyled: parsed: +Infinity% , gzip: +Infinity%
SpeedDialAction: parsed: +Infinity% , gzip: +Infinity%
@mui/joy/Select: parsed: +Infinity% , gzip: +Infinity%
@mui/joy/Menu: parsed: +Infinity% , gzip: +Infinity%
Tooltip: parsed: +Infinity% , gzip: +Infinity%
SwipeableDrawer: parsed: +Infinity% , gzip: +Infinity%
TabList: parsed: +Infinity% , gzip: +Infinity%
SpeedDial: parsed: +Infinity% , gzip: +Infinity%
Pagination: parsed: +Infinity% , gzip: +Infinity%
Alert: parsed: +Infinity% , gzip: +Infinity%
LoadingButton: parsed: +Infinity% , gzip: +Infinity%
Popover: parsed: +Infinity% , gzip: +Infinity%
Drawer: parsed: +Infinity% , gzip: +Infinity%
PaginationItem: parsed: +Infinity% , gzip: +Infinity%
and 271 more changes

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against f19f2b6

@zannager zannager requested a review from Janpot September 17, 2024 16:02
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 18, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 18, 2024
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.

Looks good. We can add a notifications demo in a separate PR. I propose we just copy https://mui.com/toolpad/core/react-use-notifications/#close-notifications

@bharatkashyap
Copy link
Member Author

Looks good. We can add a notifications demo in a separate PR. I propose we just copy https://mui.com/toolpad/core/react-use-notifications/#close-notifications

I can update the current notifications demo in this PR itself, should be a small change

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 20, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 20, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 23, 2024
@Janpot Janpot mentioned this pull request Sep 26, 2024
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.

We are getting there 👍

docs/data/material/components/app-bar/app-bar.md Outdated Show resolved Hide resolved
docs/data/material/components/app-bar/app-bar.md Outdated Show resolved Hide resolved
docs/data/material/components/breadcrumbs/breadcrumbs.md Outdated Show resolved Hide resolved

### Page Container

The [PageContainer](https://mui.com/toolpad/core/react-page-container/) component in `@toolpad/core` is the ideal wrapper for the content of your dashboard. It makes the Material UI Container navigation aware and extends it with page title, breadcrumbs, actions, and more.
The [PageContainer](https://mui.com/toolpad/core/react-page-container/) component in `@toolpad/core` is the ideal wrapper for the content of your dashboard. It makes the Material UI Container navigation-aware and extends it with page title, breadcrumbs, actions, and more.

{{"demo": "../breadcrumbs/PageContainerBasic.js", "height": 400, "hideToolbar": true}}
Copy link
Member

@oliviertassinari oliviertassinari Sep 29, 2024

Choose a reason for hiding this comment

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

The dark/light mode is broken on all those demos.

As far as I understand things, theme={demoTheme} shouldn't be provided, not be the AppProvider responsibility to have this. Imagine I already have an application configured with a theme, I want some of my pages to have a Toolpad container, this shouldn't break. I mean, https://marmelab.com/react-admin/Theming.html feels wrong, no?

At minimum, MUI System should support theme nesting where you can inherit the light/dark mode and use different values for the other values.

Copy link
Member

@oliviertassinari oliviertassinari Sep 29, 2024

Choose a reason for hiding this comment

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

const demoWindow = window !== undefined ? window() : undefined; feels confusing, I think it should copy

* Injected by the documentation to work in an iframe.

to be clear.

docs/data/material/components/snackbars/snackbars.md Outdated Show resolved Hide resolved
docs/data/material/components/snackbars/snackbars.md Outdated Show resolved Hide resolved

// preview-start
const key = online
? notifications.show('You are now online', {
Copy link
Member

@oliviertassinari oliviertassinari Sep 29, 2024

Choose a reason for hiding this comment

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

Can we change the position of the notification? It feels hard to see.

Copy link
Member Author

Choose a reason for hiding this comment

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

The only way I can see how is to do this sort of a scoped notification: https://mui.com/toolpad/core/react-use-notifications/#scoped-notifications

Copy link
Member

Choose a reason for hiding this comment

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

Then it sounds like a missing API. Creating an issue for Toolapd could be enough.

Copy link
Member

Choose a reason for hiding this comment

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

But the position is the same as the other demos. Isn't this just hard to see because of the color? Maybe it's a matter of matching the look to the other demos?

Copy link
Member Author

@bharatkashyap bharatkashyap Oct 9, 2024

Choose a reason for hiding this comment

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

But the position is the same as the other demos. Isn't this just hard to see because of the color? Maybe it's a matter of matching the look to the other demos?

Yes, to match the look we need to add an alert key to the slotProps for NotificationsProvider so that we can pass in variant: "filled" to the Alert being used in case a severity prop is passed (which is the case in this demo). I can make an issue on Toolpad for this, let me know what you think

Copy link
Member

Choose a reason for hiding this comment

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

the position is the same as the other demos. Isn't this just hard to see because of the color

Right fair, the position is correct, it's the visual design that is broken. https://notistack.com/features/basic for error looks correct;

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 30, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 1, 2024
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Oct 2, 2024
@Janpot Janpot self-requested a review October 7, 2024 09:48
@oliviertassinari
Copy link
Member

This looks much better 👍

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 14, 2024

{{"demo": "../breadcrumbs/PageContainerBasic.js", "height": 400, "hideToolbar": true}}
{{"demo": "../breadcrumbs/PageContainerBasic.js", "height": 400, "defaultExpanded": false}}
Copy link
Member

Choose a reason for hiding this comment

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

Should this be the same as the breadcrumbs page?

Suggested change
{{"demo": "../breadcrumbs/PageContainerBasic.js", "height": 400, "defaultExpanded": false}}
{{"demo": "../breadcrumbs/PageContainerBasic.js", "height": 400, "bg": "inline", "defaultExpanded": false}}


The [DashboardLayout](https://mui.com/toolpad/core/react-dashboard-layout/) component from `@toolpad/core` is the starting point for dashboarding applications. It takes care of application layout, theming, navigation, and more. An example usage of this component:

{{"demo": "DashboardLayoutBasic.js", "height": 400, "iframe": true, "hideToolbar": true}}
{{"demo": "DashboardLayoutBasic.js", "height": 400, "iframe": true, "defaultExpanded": false}}
Copy link
Member

Choose a reason for hiding this comment

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

Should this also do bg: 'inline'?

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.

Just one last suggestion

@bharatkashyap bharatkashyap merged commit 1719ed4 into mui:master Oct 15, 2024
22 checks passed
@prakhargupta1
Copy link
Member

prakhargupta1 commented Oct 15, 2024

https://deploy-preview-43796--material-ui.netlify.app/material-ui/react-dialog/#usedialogs
There seems to be an extra code block below the demo. Not sure if it is intentional.

@bharatkashyap
Copy link
Member Author

https://deploy-preview-43796--material-ui.netlify.app/material-ui/react-dialog/#usedialogs There seems to be an extra code block below the demo. Not sure if it is intentional.

A remnant of when the demo had hideToolbar set to true - I'll remove it in a separate PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation Toolpad
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[docs] Refine Toolpad sections on Material UI docs
5 participants