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.

Sign up for GitHub

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

Jump to bottom

[PageContainer] Allow full-size content #4480

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

[PageContainer] Allow full-size content #4480

wants to merge 8 commits into from
+681 −204

Conversation

apedroferreira
Copy link
Member

@apedroferreira apedroferreira commented Nov 26, 2024 •
edited
Loading

Closes #4324
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

@apedroferreira apedroferreira added core Infrastructure work going on behind the scenes bug 🐛 Something doesn't work component: PageContainer labels Nov 26, 2024
@apedroferreira apedroferreira self-assigned this Nov 26, 2024
apedroferreira
apedroferreira commented Nov 26, 2024
View reviewed changes
docs/pages/toolpad/core/api/sign-out-button.json
@@ -1,5 +1,5 @@
{
"props": {},
"props": { "children": { "type": { "name": "node" } } },
Copy link
Member Author

@apedroferreira apedroferreira Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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
apedroferreira commented Nov 26, 2024
View reviewed changes
scripts/generateProptypes.ts
@@ -204,7 +204,7 @@ async function run(argv: HandlerArgv) {
folderName = folderName.slice(9);
}

return fileName === folderName;
return !fileName.endsWith('.test');
Copy link
Member Author

@apedroferreira apedroferreira Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@apedroferreira apedroferreira Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

@apedroferreira apedroferreira changed the title [PageContainer] Allow full-size content inside [PageContainer] Allow full-size content Nov 26, 2024
@apedroferreira apedroferreira added scope: toolpad-core Abbreviated to "core" and removed core Infrastructure work going on behind the scenes labels Nov 26, 2024
@apedroferreira
Copy link
Member Author

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
Copy link
Member

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

bharatkashyap
bharatkashyap requested changes Nov 27, 2024
View reviewed changes
Copy link
Member

@bharatkashyap bharatkashyap left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
bharatkashyap
bharatkashyap approved these changes Nov 28, 2024
View reviewed changes
Copy link
Member

@bharatkashyap bharatkashyap 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 to me!

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bharatkashyap bharatkashyap bharatkashyap approved these changes

Assignees

@apedroferreira apedroferreira

Labels
bug 🐛 Something doesn't work component: PageContainer PR: out-of-date The pull request has merge conflicts and can't be merged scope: toolpad-core Abbreviated to "core"
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow PageContainer Children to Fully Utilize Viewport Height
2 participants
@apedroferreira @bharatkashyap