-
Notifications
You must be signed in to change notification settings - Fork 87
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
fix: dropdown menu rendering above headers #6501
base: develop
Are you sure you want to change the base?
Conversation
Hi @LeonardYam thanks for the contribution! Do give us some time to get on this PR! In the meantime could you rebase to |
fa12189
to
6888274
Compare
Hi @foochifa, I've just rebased the branch! I've tested the changes on my local build. Screen.Recording.2023-07-06.at.12.40.21.AM.mov |
Sorry @LeonardYam didn't see the convo in the issue #6026! Honestly, would prefer if we kept the floating-ui's portal cause:
Was perhaps looking into exploring using floating-ui's hide middleware But welcomed if you have other suggestions! If you need more help feel free to reach out too! |
Hi @foochifa, so sorry for the late reply! From what I've read, portals mainly have two use cases:
If I'm not wrong, the dropdown menu does not fall into these use cases, which was why I opted to remove the portal. On the other hand, we could also keep the portal for defensive programming etc. I'll have to explore on how we could keep the portal (either through portalling into a suitable element or using the hide middleware), let me know what approach would be preferred!
Sorry I'm quite new to webdev so I don't quite understand this point 😅. |
Heya @LeonardYam No worries, I realised most of the concern is still taken care of by Was partially worried about potential clipping of the popover (either by length of options or by number of options) without portal-ing but seems like virtualisation and our styles in Am just running the CI/CD tests now, thanks for the contribution! |
Sorry! Gimme a lil time @LeonardYam, will probably have to pick your commits into a new branch within our repo to run the chromatic tests, will create a new PR to ensure the tests pasts first 🙏 |
Hi @foochifa, no worries! Thanks for the review! Let me know if there are any issues with the tests! |
@LeonardYam Sorry for taking some time to get back to you, after discussing with UX, actually both the interaction in the screenshots are undesired. For the after screenshot, it's because there are elements that are interact-able but hidden from view. This also applies to the before screenshot, when scrolled out of view. To fix the case above, one of the ways is to use And sorry for the last minute changes to the intended behaviour in the issue. |
Hi @foochifa, no worries! Good point about the UX, i would like to continue working on this! |
Hi @foochifa, sorry for the late update! From my initial testing, the hide middleware fix works as intended for the form creation page! However, this does not work for public forms. I think that the usage of the sliding header interferes with the clipping calculations. I will investigate further and get back to you on this! |
d08d6d2
to
7a38d8d
Compare
7a38d8d
to
15c53ec
Compare
Hi @foochifa, unfortunately upon further investigation, I don't think it is possible for the hide middleware to fix the bug in the public form. As a workaround, I made a custom middleware that detects when the slide header covers the combobox (bottom-left corner specifically). Let me know if this works! Behaviour: recording1.mp4 |
Nice thanks @LeonardYam will take a look asap! |
Problem
Currently, dropdown menus are being rendered above sticky headers when scrolling. This behaviour can be seen during form creation and in public forms.
This was caused by
FloatingPortal
inSelectMenu.tsx
which portals the dropdown menu into the document body, making it render above everything else in the app.Closes #6026
Solution
Remove
FloatingPortal
, then assign thedropdown
z-index value from Chakra UI to the menu, ensuring it still renders above other elements appropriately.Side-notes:
dropdown
in Chakra UI instead of 1100 forsticky
.Breaking Changes
Before & After Screenshots
BEFORE:
AFTER:
Tests