improve dropdown menu component placement #SHIELD-1235 #2837
Conversation
… enough screen estate
…ax-menu width to body width
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: d95a1e0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 96 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -111,4 +115,72 @@ storiesOf('Components|Dropdowns|DropdownMenu', module) | |||
| </SpacingsStack> | |||
| </DropdownMenu> | |||
| </Section> | |||
| )); | |||
| )) | |||
| .add('DropdownMenu - Reposition menu if necessary', () => { | |||
There was a problem hiding this comment.
I don't think this story is needed for devs (or is it?), but I'm leaving it here for now, in case someone wants to check the positioning-logic in storybook during review.
misama-ct
changed the title
|
Thanks a lot for working on this, Michael 🙇 To be honest, it's difficult for me to follow along the changes without going too deep since it's a lot of different styles calculation due to the nature of the task. The thing that is more surprising to me in the Screen.Recording.2024-06-18.at.10.14.23.movScreen.Recording.2024-06-18.at.10.13.04.mov |
If there wasn't an 'auto' or 'scale' option, this function wouldn't be needed, because then we'd have full-control over the dropdowns dimensions. But those 2 options outsource the decision of how wide the dropdown will be to the browser (based on dropdown content) and only after the browser has rendered the dropdown, I can query for the actual width and do the proper positioning. What I could do to avoid the flickering is, hide the dropdown from the eye with |
I think using the Another thing I'd like @FilPob to confirm is the size of the menu when we use the |
The
But in any case: What I do is limit the maximum width. If the parents-width is not exceeding the viewport this behavior won't be applied. |
|
Uff, I am a bit lost with the |
Sure. Just ping me once you have time or schedule a remeet. |
|
@CarlosCortizasCT FYI @FilPob and I were checking out the previous behavior of the It was consistently creating a dropdown which was as wide as the viewport, aligned with the trigger, appearing to end on the screen-edge but actually overflowing it. The solution I proposed will - while not ideal - at least force-fit it on the screen, so it's at step forward. However, we failed to see a use-case for a window-wide dropdown. Is anything speaking against deprecating the |
Hi Michael ✋
I think this is a question for @FilPob to answer. |
|
Am also all in for removing the |
|
🥷 Code experts: no user matched threshold 10 See details
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
@CarlosCortizasCT I added the |
| @@ -3,6 +3,10 @@ import SecondaryButton from '@commercetools-uikit/secondary-button'; | |||
| import { act, screen, render } from '../../../../../test/test-utils'; | |||
| import DropdownMenu from './dropdown-menu'; | |||
|
|
|||
| // Pause test execution to wait for the DOM-operations to finish | |||
| const waitForDOMOperations = async () => | |||
There was a problem hiding this comment.
Perhaps we can use the utilities Jest already providers for dealing with timers in tests. What do you think?
There was a problem hiding this comment.
As far as I understood, those utilities are there to avoid actual waiting while running the tests (basically mocking timer-functions). But the setTimeout function here is indeed needed to defer the execution of the code following after. The value of 0 milliseconds is making sure, that execution happens immediately after the recent call stack (= calculating the final DOM in this case) is cleared, so there is no actual waiting involved and test-execution won't take any longer than before.
There was a problem hiding this comment.
Thanks for the explanation, Michael.
I understand the use case and why we need somehow managing the timeout in these tests, what I'm suggesting is that, instead of using custom functions, we can use what the testing library we use already provides.
In this case, using jest.useFakeTimers() and await jest.runAllTimersAsync(); will work and we wouldn't need to introduce custom code in the test suite.
Would that be reasonable for you?
There was a problem hiding this comment.
I see, thanks for the hint, i would've never guessed that runAllTimersAsync is the key. I went through the jest docs but couldn't find anything fitting, and the crucial part that would eventually have made me realize it is, only shows up in my editor as inline-help after putting the function in use:
I pushed the changes, please have another look. Thanky you very much!
| beforeAll(() => { | ||
| jest.useFakeTimers(); | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| jest.useRealTimers(); | ||
| }); |
There was a problem hiding this comment.
Technically this it is not needed to restore the timers.
Since Jest executes every test file in an isolated process, faking the timers here won't affect other test files.
misama-ct
deleted the
SHIELD-1235-improve-dropdown-menu-component-placement
branch
Summary
The DropdownMenu component has some flaws when it comes to displaying & positioning the dropdown-content. This pull-request attempts to resolve known issues, introducing the following logic changes:
Changes
scale, the maximum menu-width is limited to ensure it fits into the viewport and preserves the dropdown appearancesetTimeoutfunction, doing the positioning after an initial render, sincehorizontalConstraintoptions like 'auto' or 'scale' don't result in height- or width-values that can be used for computing positions.