Skip to content

Conversation

LuseBiswas
Copy link
Contributor

@LuseBiswas LuseBiswas commented Sep 5, 2024

Fixes #43572
Fixes #43636

@mui-bot
Copy link

mui-bot commented Sep 5, 2024

Netlify deploy preview

https://deploy-preview-43626--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against af7e0e6

@DiegoAndai DiegoAndai self-requested a review September 5, 2024 19:55
@DiegoAndai DiegoAndai added scope: dialog Changes related to the dialog. package: material-ui labels Sep 5, 2024
@DiegoAndai DiegoAndai changed the title Fixing Broken Scrolling in Full Screen Dilouge Box [material-ui][Dialog] Fix broken scrolling in full screen Sep 5, 2024
Copy link
Member

@DiegoAndai DiegoAndai left a 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

@LuseBiswas
Copy link
Contributor Author

@DiegoAndai
Ok, as I see that the Issue is resolved by implementing it in a Sandbox as you mentioned above.
And I learnt to how write code in less Line of Code.

So can i code get merge with main branch or I have to make some more changes ?

@DiegoAndai
Copy link
Member

@DiegoAndai
Copy link
Member

Nice @LuseBiswas. Finally, let's remove the empty lines 196 and 201

Signed-off-by: Diego Andai <[email protected]>
@DiegoAndai
Copy link
Member

DiegoAndai commented Sep 5, 2024

Thanks, @LuseBiswas; now let's wait for the checks to finish 😊. I'll do a final review tomorrow and merge.

@ZeeshanTamboli ZeeshanTamboli added type: bug It doesn't behave as expected. type: regression A bug, but worse, it used to behave as expected. labels Sep 6, 2024
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.

I believe we should add a test case as well to prevent regressions in the future.

@DiegoAndai
Copy link
Member

@ZeeshanTamboli, I added a test and confirmed that it fails without the fix. Please review 😊

@LuseBiswas
Copy link
Contributor Author

@DiegoAndai does it means that my changes in code does not fix the bug. Should I rewrite it ?

@DiegoAndai
Copy link
Member

@LuseBiswas no, it confirms that your changes fix the bug 👍🏼 no change needed.

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.

@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.

@ZeeshanTamboli ZeeshanTamboli changed the title [material-ui][Dialog] Fix broken scrolling in full screen [material-ui][Dialog] Fix broken scrolling in full screen mode Sep 7, 2024
@ZeeshanTamboli ZeeshanTamboli merged commit efc1396 into mui:master Sep 7, 2024
19 checks passed
Michael-Hutchinson pushed a commit to Michael-Hutchinson/material-ui that referenced this pull request Sep 7, 2024
…3626)

Signed-off-by: Diego Andai <[email protected]>
Co-authored-by: Diego Andai <[email protected]>
Co-authored-by: ZeeshanTamboli <[email protected]>
viliket added a commit to viliket/live-trains-finland that referenced this pull request Sep 8, 2024
This fix can be removed after a new patch of Material UI with mui/material-ui#43626 is released.
viliket added a commit to viliket/live-trains-finland that referenced this pull request Sep 8, 2024
This fix can be removed after a new patch of Material UI with mui/material-ui#43626 is released.
))}
</Dialog>,
);
const paperElement = getByTestId('paper');
Copy link
Member

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

Suggested change
const paperElement = getByTestId('paper');
const paperElement = screen.getByTestId('paper');

per point 2 of mui/mui-public#173.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: dialog Changes related to the dialog. type: regression A bug, but worse, it used to behave as expected.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[material-ui][Dialog] maxWidth exceeds in v6 [material-ui][Dialog] Broken scrolling in fullScreen dialogs (v6)
5 participants