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

[Kxiru]

… vertically

Done

• Added a scss file for Contextual Menu
• Implemented initial logic by @mas-who to vertically position the Menu optimally based on the viewport height.
• Then transferred this logic from AdjustVerticalHeight function in Contextual Menu, to updateVerticalPosition function in ContextualMenuDropdown.tsx such that the menu may be adjusted according to both the container height and the viewport height.

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

• Contextual Menu dropdown now displays "upwards" if there is not enough space downwards.

[webteam-app]

Demo

Jenkins

demos.haus

[Kxiru]

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

[huwshimi]

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

Yes please!

[Kxiru]

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]

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]

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

[Kxiru]

@huwshimi , thanks for our chat today! I've pushed the changes that we made and the written suggestions you provided. I'll let you know of my progress with creating a test.

[mas-who]

@huwshimi @Kxiru regarding creating a test for this component, I believe since react testing library uses jsdom, it will be tricky to setup a test that relies on the component render cycle and perform assertion on the final element positions. See this issue for example which has a suggestion to directly modify a dom node's prototype to mock the values but I don't think it would work for assertions if we want to check dom element position. I think maybe we can setup the test by moving updateVerticalPosition outside of the component and create a unit test for the function?

[edlerd]

@huwshimi @Kxiru regarding creating a test for this component, I believe since react testing library uses jsdom, it will be tricky to setup a test that relies on the component render cycle and perform assertion on the final element positions. See this issue for example which has a suggestion to directly modify a dom node's prototype to mock the values but I don't think it would work for assertions if we want to check dom element position. I think maybe we can setup the test by moving updateVerticalPosition outside of the component and create a unit test for the function?

If I understand your problem correctly @mas-who @Kxiru you can have a look at the updates the tooltip to fit on the screen test in Tooltip.test.jsx for a solution of a very similar problem.

[mas-who]

@huwshimi @Kxiru regarding creating a test for this component, I believe since react testing library uses jsdom, it will be tricky to setup a test that relies on the component render cycle and perform assertion on the final element positions. See this issue for example which has a suggestion to directly modify a dom node's prototype to mock the values but I don't think it would work for assertions if we want to check dom element position. I think maybe we can setup the test by moving updateVerticalPosition outside of the component and create a unit test for the function?

If I understand your problem correctly @mas-who @Kxiru you can have a look at the updates the tooltip to fit on the screen test in Tooltip.test.jsx for a solution of a very similar problem.

Hmm not sure if it's the same problem, the calculation performed for positioning the dropdown isn't really based on event listeners. As far as I can tell, what we can assert for this component test case is that it's top and bottom style attributes have been correctly calculated. Note this is due to inline style being applied. To do that, we need to get the dropdown element from the dom after rendering the component, which would have zero dimensions as discussed in the reference issue. This would cause assertion to fail.

[huwshimi]

To get this to work you'll need to mock the functions that don't have dimensions, like we do in this test: https://github.com/canonical/react-components/blob/2825eb98ed253825ef69e63c261c7c9605b15f4f/src/hooks/useWindowFitment.test.ts#L72-L78.

You'll need to do this for the scroll parent, the position node and the dropdown elements.

You'll also need to double render the component as RTL doesn't do full lifecycle rendering so we have to manually rerender the component to make the hooks run after the elements exist in the DOM:

https://github.com/canonical/react-components/blob/2825eb98ed253825ef69e63c261c7c9605b15f4f/src/components/ContextualMenu/ContextualMenuDropdown/ContextualMenuDropdown.test.tsx#L42-L44.

If that doesn't make sense let me know and I can give you a concrete example of how to construct the test.

[Kxiru]

@huwshimi , @mas-who has pushed a commit that recreates the test environment to check whether VerticalPosition resolves to top or bottom - and consequently, whether the styling attribute of the component Span has a bottom of 0, or not. Please could you let us know your thoughts of the test implementation?

[huwshimi]

@huwshimi , @mas-who has pushed a commit that recreates the test environment to check whether VerticalPosition resolves to top or bottom - and consequently, whether the styling attribute of the component Span has a bottom of 0, or not. Please could you let us know your thoughts of the test implementation?

This looks good! I think there are two other cases that need tests:

• that the menu is above if there is enough area in the window, but not in the scroll parent
• that the menu is below if there is not enough area in the window or scroll parent, but also that there is more space below than above.

I also think it could be worth splitting the current test into the two assertions to make it clear which mocks are required to make each test pass. It might be helpful to move some of the set up into a function to make the four tests cleaner.

[mas-who]

@huwshimi , @mas-who has pushed a commit that recreates the test environment to check whether VerticalPosition resolves to top or bottom - and consequently, whether the styling attribute of the component Span has a bottom of 0, or not. Please could you let us know your thoughts of the test implementation?

This looks good! I think there are two other cases that need tests:

• that the menu is above if there is enough area in the window, but not in the scroll parent
• that the menu is below if there is not enough area in the window or scroll parent, but also that there is more space below than above.

I also think it could be worth splitting the current test into the two assertions to make it clear which mocks are required to make each test pass. It might be helpful to move some of the set up into a function to make the four tests cleaner.

@huwshimi I've made those changes as mentioned :+1:

[github-actions[bot]]

:tada: This PR is included in version 1.1.0 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: