canonical / react-components

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

chore: Allow Contextual Menu Component to be positioned optimally [WD-10977] #1101

Open Kxiru opened 2 weeks ago

Kxiru commented 2 weeks ago

… vertically

Done

QA

Storybook

To see rendered examples of all react-components, run:

yarn start

QA in your project

from react-components run:

yarn build
npm pack

Install the resulting tarball in your project with:

yarn add <path-to-tarball>

Percy steps

N/A

Fixes

image

webteam-app commented 2 weeks ago

Demo

Jenkins

demos.haus

github-actions[bot] commented 2 weeks ago

This PR is being prevented from merging because it needs to be reviewed on Percy.

Go to Percy, find the build relevant to this PR and check if it looks as expected.

Once it's approved, add the label Review: Percy +1 to this PR.

Kxiru commented 2 weeks ago

Thanks @huwshimi for your review. Are you saying that the implemented logic would be better placed / integrated into updateVerticalPosition?

huwshimi commented 2 weeks ago

Thanks @huwshimi for your review. Are you saying that the implemented logic would be better placed / integrated into updateVerticalPosition?

Yes please!

Kxiru commented 6 days ago

This component already has support for adjusting the dropdown to be above or below, so there's no need to add additional state. Instead, you can just change the calculation from being relative to the scroll parent, to being relative to the window here:

https://github.com/canonical/react-components/blob/9668ce671a76d74cb3d7f1153011ad2d7c73062d/src/components/ContextualMenu/ContextualMenuDropdown/ContextualMenuDropdown.tsx#L239

Don't we want the component to be able to react according to BOTH the scroll parent and the window as well?

huwshimi commented 6 days ago

Don't we want the component to be able to react according to BOTH the scroll parent and the window as well?

Are you thinking of a scenario where the menu is at the bottom of a scrolling container, but that container is not at the bottom of the screen, so you want the menu to flip to above the button?

If that's the behaviour you want then you could add a second rect that is relative to the window and then adjust this calculation:

https://github.com/canonical/react-components/blob/9668ce671a76d74cb3d7f1153011ad2d7c73062d/src/components/ContextualMenu/ContextualMenuDropdown/ContextualMenuDropdown.tsx#L250-L252

This would use both rects to calculate "dropdown fits below in scroll parent" AND "dropdown fits below in window", in pseudo code this could be something like:

    setVerticalPosition(
      scrollParentSpaceBelow >= dropdownHeight  && windowSpaceBelow >= dropdownHeight || windowSpaceBelow > spaceAbove ? "bottom" : "top"
    );
Kxiru commented 3 days ago

@huwshimi Hi there! I have actioned your suggestions. Please let me know if this is in line with what you were thinking :).