[PageContainer] Allow full-size content by apedroferreira · Pull Request #4480 · mui/toolpad

apedroferreira · 2024-11-26T19:45:03Z

Closes #4324
Closes #4072
Needed it already in the CRUD for the list table to take up the full space in the layout.

As per the discussion in #4324:

Allow full-size content inside PageContainer with flex: 1 or height: 100%.
Add sx prop to PageContainer.
Separate PageHeader from PageContainer to optionally be used as a standalone component
Documented both the cases above

https://deploy-preview-4480--mui-toolpad-docs.netlify.app/toolpad/core/react-page-container

BREAKING CHANGE: toolbar slot was moved to PageHeader component which can be configured with the header slot in PageContainer. Updated examples in docs to reflect this. It should just be a step towards separating the components as proposed in #4525.

apedroferreira · 2024-11-26T19:48:06Z

docs/pages/toolpad/core/api/sign-out-button.json

@@ -1,5 +1,5 @@
 {
-  "props": {},
+  "props": { "children": { "type": { "name": "node" } } },


Proptypes are now being generated for the components which don't match the name of the folder (secondary components), I think that should be a good thing?
Changed it in the script that generates proptypes so that the API documentation for PageHeader would be there, auto-generated.

apedroferreira · 2024-11-26T19:51:28Z

scripts/generateProptypes.ts

      }

-      return fileName === folderName;
+      return !fileName.endsWith('.test');


Generates proptypes for all non-test files inside the folder of each component.

This is also generating proptypes for components marked as @ignore in the JSDoc comments - something to fix?

~~I guess we can try to exclude them if it's easy to do.~~
done!

…ntainer

apedroferreira · 2024-11-26T23:35:26Z

Looks like the CI is having out of memory issues again, we probably either need to manage dependencies to stay under or increase instance sizes?

bharatkashyap · 2024-11-27T12:33:12Z

Looks like the CI is having out of memory issues again, we probably either need to manage dependencies to stay under or increase instance sizes?

Yes, although I'm wondering what exactly triggered the CI out of memory issues

docs/data/toolpad/core/components/page-container/page-container.md

bharatkashyap

Looks good, just some suggestions on the docs demo and one flag on the proptypes script.

Also, I'm guessing this counts as a breaking change since we are renaming PageContainerToolbar to PageHeaderToolbar

docs/data/toolpad/core/components/page-container/PageContainerHeader.tsx

…r.md Co-authored-by: Bharat Kashyap <[email protected]> Signed-off-by: Pedro Ferreira <[email protected]>

…ntainer

bharatkashyap

On second thought:

Apart from standalone usage, should we make the PageHeader composable inside the PageContainer as well? Just as we made the ThemeSwitcher composable inside ToolbarActions?

Something like

<PageContainer>
  <PageHeader slots={{toolbar: CustomToolbar}} />
  {children}
</PageContainer>

This way, we could also move the toolbar slot to PageHeader, which better reflects its position anyway.

Wdyt? I know it's a tangential part of this PR, so feel free to extract the rest if this feels too blocking

apedroferreira · 2024-12-02T13:21:25Z

On second thought:

Apart from standalone usage, should we make the PageHeader composable inside the PageContainer as well? Just as we made the ThemeSwitcher composable inside ToolbarActions?

Something like
<PageContainer>
  <PageHeader slots={{toolbar: CustomToolbar}} />
  {children}
</PageContainer>
This way, we could also move the toolbar slot to PageHeader, which better reflects its position anyway.

Wdyt? I know it's a tangential part of this PR, so feel free to extract the rest if this feels too blocking

Sounds like a probably good idea, we can discuss it just to make sure.
The only drawback from that suggestion would be that the PageContainer wouldn't work as well out of the box, but we could probably fix that by giving it a slot for the header content with PageHeader as the default?

bharatkashyap · 2024-12-02T13:41:26Z

Sounds like a probably good idea, we can discuss it just to make sure. The only drawback from that suggestion would be that the PageContainer wouldn't work as well out of the box, but we could probably fix that by giving it a slot for the header content with PageHeader as the default?

I feel we can enforce a migration now to a composable API and remove the header and toolbar slots entirely given that it is likely to be the more popular way of customization. The breaking change would have to be adding a:

<PageContainer>
+  <PageHeader>
+     <PageHeaderBreadcrumbs /> // would contain the default breadcrumbs
+    <PageHeaderToolbar> ... </PageHeaderToolbar>// (optional, this would replace the `toolbar` slot)
+  </PageHeader>
   {children}
 </PageContainer>

to all uses of PageContainer to get the breadcrumbs out of the box.

We can discuss this tomorrow or in next week's grooming 👍🏻

Janpot · 2024-12-03T22:23:12Z

What would be the difference between PageContainer and

On second thought:

Apart from standalone usage, should we make the PageHeader composable inside the PageContainer as well? Just as we made the ThemeSwitcher composable inside ToolbarActions?

Something like
<PageContainer>
  <PageHeader slots={{toolbar: CustomToolbar}} />
  {children}
</PageContainer>
This way, we could also move the toolbar slot to PageHeader, which better reflects its position anyway.

Wdyt? I know it's a tangential part of this PR, so feel free to extract the rest if this feels too blocking

what would be the difference between the PageContainer and a @mui/material Container? Wouldn't it be enough to just build a PageHeader component?

apedroferreira · 2024-12-09T10:12:21Z

Sounds like a probably good idea, we can discuss it just to make sure. The only drawback from that suggestion would be that the PageContainer wouldn't work as well out of the box, but we could probably fix that by giving it a slot for the header content with PageHeader as the default?

I feel we can enforce a migration now to a composable API and remove the header and toolbar slots entirely given that it is likely to be the more popular way of customization. The breaking change would have to be adding a:
<PageContainer>
+  <PageHeader>
+     <PageHeaderBreadcrumbs /> // would contain the default breadcrumbs
+    <PageHeaderToolbar> ... </PageHeaderToolbar>// (optional, this would replace the `toolbar` slot)
+  </PageHeader>
   {children}
 </PageContainer>
to all uses of PageContainer to get the breadcrumbs out of the box.

We can discuss this tomorrow or in next week's grooming 👍🏻

One of the main advantages of using Toolpad is that it does standard things for you with little code. I feel like if we separate the container into too many sub-components that usually need to be used it kind of complicates things.

I prefer the approach where we provide a few components and more detailed customization can be made with slots.

what would be the difference between the PageContainer and a @mui/material Container? Wouldn't it be enough to just build a PageHeader component?

Not a lot right now, but at the moment the PageContainer has styles that make it so that the content inside it can take up the full height inside the layout, and some default spacing around its content.

It seems like these wouldn't be difficult to do with a standard Container, especially if we provide an example on the docs on how to do full-size content in a page... So we could export the PageHeader only and include an example like that in its documentation.

apedroferreira · 2024-12-09T12:14:24Z

As discussed, the PageContainer will have a header slot for now that uses PageHeader by default.
Also as discussed, we can eventually deprecate the PageContainer and have a PageHeader only. Created an issue for that in #4525

Also, as a global discussion, we can start considering a more composable API for customization with different sub-components as opposed to slots in some cases at least.

bharatkashyap

Looks good! We should make sure to mark this as a breaking change in the release notes

docs/data/toolpad/core/components/page-container/ActionsPageContainer.js

docs/data/toolpad/core/components/page-container/ActionsPageContainer.tsx

…ntainer.tsx Co-authored-by: Bharat Kashyap <[email protected]> Signed-off-by: Pedro Ferreira <[email protected]>

…ntainer.js Co-authored-by: Bharat Kashyap <[email protected]> Signed-off-by: Pedro Ferreira <[email protected]>

Allow full-size content in PageContainer

aa8a72c

apedroferreira added internal Behind-the-scenes enhancement. Formerly called “core”. type: bug It doesn't behave as expected. component: PageContainer labels Nov 26, 2024

apedroferreira self-assigned this Nov 26, 2024

apedroferreira commented Nov 26, 2024

View reviewed changes

apedroferreira added 2 commits November 26, 2024 19:56

Merge remote-tracking branch 'upstream/master' into full-size-page-co…

95a3bc5

…ntainer

huh

1b947b2

apedroferreira changed the title ~~[PageContainer] Allow full-size content inside~~ [PageContainer] Allow full-size content Nov 26, 2024

apedroferreira added scope: toolpad-core Abbreviated to "core" and removed internal Behind-the-scenes enhancement. Formerly called “core”. labels Nov 26, 2024

apedroferreira requested a review from bharatkashyap November 26, 2024 23:34

bharatkashyap reviewed Nov 27, 2024

View reviewed changes

docs/data/toolpad/core/components/page-container/page-container.md Outdated Show resolved Hide resolved

bharatkashyap requested changes Nov 27, 2024

View reviewed changes

docs/data/toolpad/core/components/page-container/PageContainerHeader.tsx Outdated Show resolved Hide resolved

apedroferreira and others added 5 commits November 27, 2024 14:10

Update docs/data/toolpad/core/components/page-container/page-containe…

d0e75c4

…r.md Co-authored-by: Bharat Kashyap <[email protected]> Signed-off-by: Pedro Ferreira <[email protected]>

Fix preview

1348a96

Merge remote-tracking branch 'upstream/master' into full-size-page-co…

b5b35e8

…ntainer

Skip proptype generation in internal components

6f2cd96

Adjust script

6b33f8d

apedroferreira requested a review from bharatkashyap November 27, 2024 19:46

This comment was marked as outdated.

Sign in to view

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Nov 28, 2024

bharatkashyap requested changes Nov 28, 2024

View reviewed changes

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

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Dec 3, 2024

Use slot for page header

1ece83d

apedroferreira mentioned this pull request Dec 9, 2024

[PageContainer] Replace PageContainer component with just PageHeader #4525

Open

Merge remote-tracking branch 'upstream' into full-size-page-container

9a65df1

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

apedroferreira added 2 commits December 9, 2024 12:34

dedoop

ba70b8c

Fix title

17d1597

apedroferreira requested a review from bharatkashyap December 9, 2024 14:00

Merge branch 'master' into full-size-page-container

c26bbeb

bharatkashyap approved these changes Dec 11, 2024

View reviewed changes

docs/data/toolpad/core/components/page-container/ActionsPageContainer.js Outdated Show resolved Hide resolved

docs/data/toolpad/core/components/page-container/ActionsPageContainer.tsx Outdated Show resolved Hide resolved

apedroferreira and others added 3 commits December 12, 2024 13:06

Update docs/data/toolpad/core/components/page-container/ActionsPageCo…

795c4f4

…ntainer.tsx Co-authored-by: Bharat Kashyap <[email protected]> Signed-off-by: Pedro Ferreira <[email protected]>

Update docs/data/toolpad/core/components/page-container/ActionsPageCo…

aa1ba41

…ntainer.js Co-authored-by: Bharat Kashyap <[email protected]> Signed-off-by: Pedro Ferreira <[email protected]>

Merge branch 'master' into full-size-page-container

d9b2fff

apedroferreira merged commit 46a3e8c into mui:master Dec 12, 2024

apedroferreira deleted the full-size-page-container branch December 12, 2024 13:43

Uh oh!

Conversation

apedroferreira commented Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apedroferreira Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apedroferreira Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharatkashyap Nov 27, 2024

Choose a reason for hiding this comment

Uh oh!

apedroferreira Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apedroferreira commented Nov 26, 2024

Uh oh!

bharatkashyap commented Nov 27, 2024

Uh oh!

Uh oh!

bharatkashyap left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

bharatkashyap left a comment

Choose a reason for hiding this comment

Uh oh!

apedroferreira commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bharatkashyap commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Janpot commented Dec 3, 2024

Uh oh!

apedroferreira commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apedroferreira commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bharatkashyap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

apedroferreira commented Nov 26, 2024 •

edited

Loading

apedroferreira Nov 26, 2024 •

edited

Loading

apedroferreira Nov 26, 2024 •

edited

Loading

apedroferreira Nov 27, 2024 •

edited

Loading

bharatkashyap left a comment •

edited

Loading

apedroferreira commented Dec 2, 2024 •

edited

Loading

bharatkashyap commented Dec 2, 2024 •

edited

Loading

apedroferreira commented Dec 9, 2024 •

edited

Loading

apedroferreira commented Dec 9, 2024 •

edited

Loading