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

[base-ui][Popup] Popup no longer opens outside viewport #39827

Merged
merged 6 commits into from
Dec 22, 2023

Conversation

adamhylander
Copy link
Contributor

Popup should no longer be outside the viewport

Closes #39818

@mui-bot
Copy link

mui-bot commented Nov 10, 2023

Netlify deploy preview

@material-ui/unstyled: parsed: +0.37% , gzip: +0.49%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 73af31d

@ZeeshanTamboli ZeeshanTamboli added bug 🐛 Something doesn't work component: Popper The React component. See <Popup> for the latest version. package: base-ui Specific to @mui/base labels Nov 10, 2023
@michaldudak
Copy link
Member

Well done, thanks!
Two more things remain to be done before we can merge it: tests and docs. Could you add a unit test that verifies the behavior? Take one of the existing ones (such as "should respect the RTL setting" or "should flip placement when edge is reached") as a reference.
As for the docs, they currently mention two middleware functions used by default (docs\data\base\components\popup\popup.md, last section). Could you please add a short description about the newly added one?

@adamhylander adamhylander force-pushed the Popup-outside-viewport branch 2 times, most recently from 2c41a7a to f01f7d0 Compare November 27, 2023 22:55
@adamhylander
Copy link
Contributor Author

Hey @michaldudak,

Thank you so much for the detailed response. I fixed the docs and added a test for the shift function. I also noticed a mistake in the rtl test. The line was expect(popup.getBoundingClientRect().right).to.equal(popup.getBoundingClientRect().right); which is always true. I changed it to expect(popup.getBoundingClientRect().right).to.equal(anchor.getBoundingClientRect().right);. However, I can't seem to pass the test_browser-1 test. It is now failing on both the rtl test and my test, the reason of which I don't quite understand. Can you perhaps take a look at this? I would be happy to tell you more about my thought process on the shift test.

@adamhylander
Copy link
Contributor Author

ping @michaldudak

@ZeeshanTamboli ZeeshanTamboli changed the title [Popup][base-ui] Popup no longer opens outside viewport [base-ui][Popup] Popup no longer opens outside viewport Dec 22, 2023
@ZeeshanTamboli
Copy link
Member

However, I can't seem to pass the test_browser-1 test. It is now failing on both the rtl test and my test, the reason of which I don't quite understand.

@adamhylander Regarding the RTL browser test, the assertion is definitely wrong as you mentioned, but I couldn't fix it. I reverted the change for now. It's not related to this PR, so let's address it separately. cc @michaldudak
For this PR's test, it should check for equality between the popup and body's left position property. It was mistakenly using to.above, possibly a typo.

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.

@adamhylander Thanks for your contribution!

@ZeeshanTamboli ZeeshanTamboli merged commit 0d62bab into mui:master Dec 22, 2023
22 checks passed
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Jan 9, 2024
Co-authored-by: adahy344 <adahy344@student.liu.se>
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: Popper The React component. See <Popup> for the latest version. package: base-ui Specific to @mui/base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Popup][base-ui] Popup does not open within the viewport
4 participants