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

[docs][base] Fix Nested modal demo positioning #37506

Merged
merged 11 commits into from
Jul 8, 2023
Merged

[docs][base] Fix Nested modal demo positioning #37506

merged 11 commits into from
Jul 8, 2023

Conversation

gitstart
Copy link
Contributor

@gitstart gitstart commented Jun 5, 2023 •
edited by ZeeshanTamboli
Loading

Fixes #37330

This code was written and reviewed by GitStart Community. Growing great engineers, one PR at a time.

Preview: https://deploy-preview-37506--material-ui.netlify.app/base-ui/react-modal/#nested-modal

@mui-bot
Copy link

mui-bot commented Jun 5, 2023 •
edited
Loading

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 465155a

oliviertassinari
oliviertassinari previously requested changes Jun 5, 2023
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

See #37330 (comment)

packages/mui-base/src/FocusTrap/FocusTrap.tsx Outdated Show resolved Hide resolved
@gitstart gitstart marked this pull request as ready for review June 7, 2023 16:59
@zannager zannager added component: modal This is the name of the generic UI component, not the React module! component: FocusTrap The React component. labels Jun 8, 2023
@zannager zannager requested a review from mnajdova June 8, 2023 07:54
@gitstart gitstart requested a review from oliviertassinari June 9, 2023 10:59
@PatrykKuniczak
Copy link

@gitstart Are you working on that task?

@gitstart
Copy link
Contributor Author

@gitstart Are you working on that task?

Yes I am @PatrykKuniczak

@PatrykKuniczak
Copy link

@gitstart Are you working on that task?

Yes I am @PatrykKuniczak

Yeeah, i see yesterday :)

@ZeeshanTamboli
Copy link
Member

@gitstart Will you continue working on this?

@gitstart
Copy link
Contributor Author

gitstart commented Jul 6, 2023

@gitstart Will you continue working on this?

This is currently awaiting review @ZeeshanTamboli

ZeeshanTamboli
ZeeshanTamboli requested changes Jul 6, 2023
View reviewed changes
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.

Thank you for the PR. In the Nested Modal component demo, when I open the modal, the page scrolls to the top. Can you please try to fix this?

packages/mui-base/src/FocusTrap/FocusTrap.tsx Outdated Show resolved Hide resolved
@ZeeshanTamboli ZeeshanTamboli changed the title MUI-37330 - [Modal, Focus Trap] Nested modal and Focus trap, wrong sliding [docs][Modal][base] Fix Nested modal demo positioning Jul 6, 2023
@ZeeshanTamboli ZeeshanTamboli added bug 🐛 Something doesn't work docs Improvements or additions to the documentation regression A bug, but worse and removed component: FocusTrap The React component. labels Jul 6, 2023
@ZeeshanTamboli ZeeshanTamboli changed the title [docs][Modal][base] Fix Nested modal demo positioning [docs][base] Fix Nested modal demo positioning Jul 6, 2023
ZeeshanTamboli
ZeeshanTamboli reviewed Jul 7, 2023
View reviewed changes
Copy link
Member

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

It looks good! Can you fix the CI by running yarn docs:typescript:formatted and then push the changes?

@gitstart @seunexplicit
fix failing ci
465155a 
Co-authored-by: seunexplicit <[email protected]>
@gitstart
Copy link
Contributor Author

gitstart commented Jul 7, 2023

It looks good! Can you fix the CI by running yarn docs:typescript:formatted and then push the changes?

Done @ZeeshanTamboli

ZeeshanTamboli
ZeeshanTamboli approved these changes Jul 8, 2023
View reviewed changes
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.

@gitstart Thanks a lot! Good find.

@ZeeshanTamboli ZeeshanTamboli added the package: base-ui Specific to @mui/base label Jul 8, 2023
@ZeeshanTamboli ZeeshanTamboli merged commit af088e2 into mui:master Jul 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZeeshanTamboli ZeeshanTamboli ZeeshanTamboli approved these changes

@mnajdova mnajdova Awaiting requested review from mnajdova

@oliviertassinari oliviertassinari Awaiting requested review from oliviertassinari

Assignees
No one assigned
Labels
bug 🐛 Something doesn't work component: modal This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation package: base-ui Specific to @mui/base regression A bug, but worse
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Modal] Nested modal demo wrong scroll
6 participants
@gitstart @mui-bot @PatrykKuniczak @ZeeshanTamboli @oliviertassinari @zannager