-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[material-ui][Dialog] Fix broken scrolling in full screen mode #43626
Conversation
Netlify deploy previewhttps://deploy-preview-43626--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this @LuseBiswas! 🚀
Here's my initial review.
As a general comment, it's good to include before and after demos so we know that the PR is actually fixing the issue.
This would be the before demo: https://codesandbox.io/p/sandbox/wizardly-swirles-yw2t4t
And for the after demo, you can clone the before one and make this change in package.json
:
-"@mui/icons-material": "latest",
+"@mui/icons-material": "https://pkg.csb.dev/mui/material-ui/commit/285afa8d/@mui/icons-material",
-"@mui/material": "latest",
+"@mui/material": "https://pkg.csb.dev/mui/material-ui/commit/285afa8d/@mui/material"
Where 285afa8d
are the first 8 characters of the commit to test (in this case 285afa8). I created the after demo for you this time 😊: https://codesandbox.io/p/sandbox/keen-dewdney-77h2xx?file=%2Fpackage.json%3A5%2C5-6%2C89&workspaceId=836800c1-9711-491c-a89b-3ac24cbd8cd8
@DiegoAndai So can i code get merge with main branch or I have to make some more changes ? |
@LuseBiswas you need to make the changes from my review: |
Nice @LuseBiswas. Finally, let's remove the empty lines 196 and 201 |
Signed-off-by: Diego Andai <[email protected]>
Thanks, @LuseBiswas; now let's wait for the checks to finish 😊. I'll do a final review tomorrow and merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should add a test case as well to prevent regressions in the future.
@ZeeshanTamboli, I added a test and confirmed that it fails without the fix. Please review 😊 |
@DiegoAndai does it means that my changes in code does not fix the bug. Should I rewrite it ? |
@LuseBiswas no, it confirms that your changes fix the bug 👍🏼 no change needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DiegoAndai @LuseBiswas I pushed a commit to refactor the test. The PR looks good, so I'll merge it. This also fixes #43636. Thanks for working on it.
…3626) Signed-off-by: Diego Andai <[email protected]> Co-authored-by: Diego Andai <[email protected]> Co-authored-by: ZeeshanTamboli <[email protected]>
This fix can be removed after a new patch of Material UI with mui/material-ui#43626 is released.
This fix can be removed after a new patch of Material UI with mui/material-ui#43626 is released.
))} | ||
</Dialog>, | ||
); | ||
const paperElement = getByTestId('paper'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the future, it should be
const paperElement = getByTestId('paper'); | |
const paperElement = screen.getByTestId('paper'); |
per point 2 of mui/mui-public#173.
Fixes #43572
Fixes #43636