canonical / react-components

A set of components based on Vanilla Framework
https://canonical.github.io/react-components
95 stars 51 forks source link

fix: contextual menu container overflow #1000

Closed petermakowski closed 9 months ago

petermakowski commented 9 months ago

Done

This pull request fixes the menu position when scrolling in overflowing containers.

Screenshots

Before

Google Chrome screenshot 001100

After

Google Chrome screenshot 001099

QA

QA steps

Fixes

Fixes: https://warthogs.atlassian.net/browse/MAASENG-2407 https://github.com/canonical/react-components/issues/687 https://github.com/canonical/react-components/issues/769

huwshimi commented 9 months ago

Hi @petermakowski, this was using a portal so that it could escape its container to prevent issues such as overflow: hidden on a parent (same as we do for tooltips, portals etc.) With this change you can see that the menus get cut off in the storybook examples:

Screenshot 2023-11-23 at 9 45 58 am Screenshot 2023-11-23 at 9 51 48 am

Any ideas for mitigating that issue?

petermakowski commented 9 months ago

Hi @petermakowski, this was using a portal so that it could escape its container to prevent issues such as overflow: hidden on a parent (same as we do for tooltips, portals etc.) Any ideas for mitigating that issue?

@huwshimi Thanks for chipping in. Very good point, clearly I was missing some context here. I intended for this to be fixed by removing overflow: hidden from a container, but this might not work in all cases.

In the examples that you pasted specifically this can be worked around by using automatic positioning and displaying the menu at the top of a trigger when not enough space is available at the bottom (the approach that we're exploring in MAAS currently). Having said that, it's a big change in approach and something that not all teams may want to adopt. I'll give this some more thought. Marking this as do not merge for now.

huwshimi commented 9 months ago

Hi @petermakowski, this was using a portal so that it could escape its container to prevent issues such as overflow: hidden on a parent (same as we do for tooltips, portals etc.) Any ideas for mitigating that issue?

@huwshimi Thanks for chipping in. Very good point, clearly I was missing some context here. I intended for this to be fixed by removing overflow: hidden from a container, but this might not work in all cases.

In the examples that you pasted specifically this can be worked around by using automatic positioning and displaying the menu at the top of a trigger when not enough space is available at the bottom (the approach that we're exploring in MAAS currently). Having said that, it's a big change in approach and something that not all teams may want to adopt. I'll give this some more thought. Marking this as do not merge for now.

Thanks Peter, we've definitely had cases in the past where we couldn't control the overflow which is why we ended up using portals, but maybe there's another approach we haven't considered.

petermakowski commented 9 months ago

@huwshimi This is ready for another review. I reverted changes related to relative positioning, keeping node position in sync with the container is now handled similarly to window resize.