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

[Grid] Prevent crash if spacing is set to zero in theme #33777

Merged
merged 7 commits into from Aug 5, 2022
Merged

[Grid] Prevent crash if spacing is set to zero in theme #33777

merged 7 commits into from Aug 5, 2022

Conversation

PunitSoniME
Copy link
Contributor

@PunitSoniME PunitSoniME commented Aug 3, 2022

Resolves #33771

Preview - https://deploy-preview-33777--material-ui.netlify.app/material-ui/customization/density/

More details in #33771 (comment).


Before: https://codesandbox.io/s/admiring-dirac-ctu7c1.

After: https://codesandbox.io/s/boring-agnesi-l7fmw1

@mui-bot
Copy link

mui-bot commented Aug 3, 2022

Details of bundle changes

Generated by 🚫 dangerJS against a515452

@PunitSoniME
Copy link
Contributor Author

@siriwatknp

Can you review this PR please ?

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@PunitSoniME Thanks for the contribution. See my comment #33771 (comment) for better understanding as to why it is happening.

We will also need to add a test in Grid (Check the CodeSandbox in the above linked comment).

docs/data/material/customization/density/DensityTool.js Outdated Show resolved Hide resolved
docs/data/material/customization/density/DensityTool.js Outdated Show resolved Hide resolved
docs/data/material/customization/density/DensityTool.js Outdated Show resolved Hide resolved
@ZeeshanTamboli ZeeshanTamboli added bug 🐛 Something doesn't work docs Improvements or additions to the documentation regression A bug, but worse labels Aug 4, 2022
@ZeeshanTamboli ZeeshanTamboli changed the title [docs] demo density tool crashes [docs] Fix demo density tool crashing Aug 4, 2022
packages/mui-material/src/Grid/Grid.js Outdated Show resolved Hide resolved
packages/mui-material/src/Grid/Grid.js Outdated Show resolved Hide resolved
packages/mui-material/src/Grid/Grid.js Outdated Show resolved Hide resolved
@ZeeshanTamboli ZeeshanTamboli added the PR: needs test The pull request can't be merged label Aug 4, 2022
@ZeeshanTamboli ZeeshanTamboli changed the title [docs] Fix demo density tool crashing [docs] Fix customizing density demo crashing Aug 4, 2022
@ZeeshanTamboli ZeeshanTamboli removed the PR: needs test The pull request can't be merged label Aug 4, 2022
@siriwatknp siriwatknp changed the title [docs] Fix customizing density demo crashing [Grid] Prevent crash if spacing is set to zero in theme Aug 4, 2022
@siriwatknp siriwatknp added the component: Grid The React component. label Aug 4, 2022
@ZeeshanTamboli
Copy link
Member

@siriwatknp @PunitSoniME I have added a test and updated the PR description explaining the issue in #33771 (comment). Please take a look.

@siriwatknp siriwatknp removed the regression A bug, but worse label Aug 4, 2022
@siriwatknp
Copy link
Member

siriwatknp commented Aug 4, 2022

I am changing the PR title to Grid instead. Ideally, this PR should be split into 2 @PunitSoniME thanks for fixing it.

@PunitSoniME
Copy link
Contributor Author

PunitSoniME commented Aug 4, 2022

Thanks @ZeeshanTamboli
I was thinking to add suggested test case, however you did it ( I owe you one 🙂 )
You helped me a lot in my recent PRs

Thanks @siriwatknp
I will take care of this next time 🙂

@propG

This comment was marked as spam.

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👏 Thank you so much @PunitSoniME @ZeeshanTamboli

@ZeeshanTamboli ZeeshanTamboli merged commit 67e1579 into mui:master Aug 5, 2022
daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
* [docs] demo density tool crashes

* [docs] prettier executed

* [docs] code changed as per suggestions

* add test

Co-authored-by: ZeeshanTamboli <zeeshan.tamboli@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: Grid The React component. docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[docs] Demo DensityTool crashes
5 participants