Popover, CustomGradientPicker, Dropdown: Fix positioning of popover when used in a dropdown #41361
Conversation
…hen used in a dropdown
andrewserong
added
[Package] Components
| @@ -75,7 +75,7 @@ export function getGradientAstWithControlPoints( | |||
| return { | |||
| length: { | |||
| type: '%', | |||
| value: position.toString(), | |||
| value: position?.toString(), | |||
There was a problem hiding this comment.
This was throwing errors in Storybook due to position being undefined. To prevent those errors, I've added in optional chaining, which seemed to do the trick. It doesn't appear to change anything in real world usage in the editor, but made it easier to log out values while debugging in storybook.
There was a problem hiding this comment.
When will it error in storybook? Are there reproduction steps?
There was a problem hiding this comment.
It was erroring when the component is first loaded if you navigate to ?path=/story/components-customgradientpicker--default and open up the console. I think in the story for this component, when it's initially mounted it doesn't yet have any position settings which caused the issue. We should probably also update the story, but making this function a little more tolerant to empty values was a quick win 🙂
| @@ -194,6 +194,7 @@ const Popover = ( | |||
| shift( { | |||
| crossAxis: true, | |||
| limiter: limitShift(), | |||
| padding: 1, // Necessary to avoid flickering at the edge of the viewport. | |||
There was a problem hiding this comment.
I'm not sure what it is about the particular settings we're using, but I couldn't get everything to work correctly (removed maxWidth on the size middleware, and a popover at the edge of the screen) without adding at least 1px of padding to the shift middleware. Otherwise We'd see jiggling like in this comment: #41268 (comment)
I'm hoping that adding 1px padding here (which only gets applied at the edge of the viewport) will be viable in the shorter-term. Though it'd be good to get to the bottom of why the issue occurs without the padding.
|
Size Change: +13 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
@kevin940726 I've added you as a reviewer, since I saw you ran into a similar issue with the Tooltip / maxWidth issue 🙂 |
| @@ -75,7 +75,7 @@ export function getGradientAstWithControlPoints( | |||
| return { | |||
| length: { | |||
| type: '%', | |||
| value: position.toString(), | |||
| value: position?.toString(), | |||
There was a problem hiding this comment.
When will it error in storybook? Are there reproduction steps?
| anchorRef={ | ||
| ! hasAnchorRef | ||
| ? containerRef?.current?.firstChild // Anchor to the rendered toggle. | ||
| : undefined | ||
| } |
There was a problem hiding this comment.
This always feels weird to me that the prop is called anchorRef but it can also accept DOM elements 😆.
There was a problem hiding this comment.
Thank you @andrewserong , code changes LGTM (and the issues is fixed)
I wonder if we could add any unit tests that will help us to avoid this regression in the future? (although not a blocker for this PR if it becomes too complicated).
Also, while testing the fix to CustomGradientPicker in Storybook, I noticed a couple of other unrelated issues:
Focus Management
It looks like the "gradient bar" is blurred when the component is mounted. As a result, clicking on the gradient bar opens the ColorPicker in a Popover that is also blurred out.
customgradientpicker-no-focus.mp4
Position and "clickability" of color stops
I also noticed that, when clicking on the gradient bar to add a color stop:
- it's impossible to add a color stop in proximity of the pre-existing start/end color stops — the "+" simply doesn't appear and clicking doesn't do anything (although I'm not sure if this is by design)
- when the "+" icon appears on hover the gradient bar, I noticed that the icon's position doesn't track perfectly the cursor. This behaviour is more visible as the cursor moves away from the middle and towards the left or the right of the gradient bar. It gets to a point where the "+" icon appears, but clicking with the mouse doesn't fire on the icon (because of its offset)
customgradientpicker-clicking-color-stops.mp4
I'll wait for confirmation of other folks with regard to these potential bugs, and then I'll proceed to open any potential issues
|
Thanks for the reviews, folks! I'll merge this in shortly.
That's a great idea — I couldn't quite think of how to neatly unit test this, though perhaps (particularly for the Tooltip) we could look at adding tests that open a Popover close to the edge of a viewport and check that the width setting is still correct. It'd be good for us to look into some of these things separately to see what's viable!
I noticed these things too. If you wouldn't mind opening up issues for them, that'd be great! I think part of the issue is with the calculation of the position of the control points — the component that's used to measure the length of the bar has some right padding or right margin that means the calculation for the |
andrewserong
deleted the
fix/dropdown-popover-anchor-reference-and-gradient-picker-popover
branch
Yeah, that's what I had in mind — triggering the opening of the popover next to the edges of the viewport. Although I'm not sure if that's possible in an environment like
I also got the same gut feeling (at least for #41390) — thank you! |
|
I've opened a follow-up for the control and insertion point positioning in: #41492 |
…hen used in a dropdown (#41361) * Popover, CustomGradientPicker, Dropdown: Fix positioning of popover when used in a dropdown * Add changelog entry
What?
Fixes: #41353
Fix positioning of the Popover when used in dropdowns and the custom gradient picker. Also, fix the issue where the width of the Popover was being reduced when it appears close to the edge of the viewport.
Why?
As reported in #41353 there were a few issues with the Popover positions when used in the custom gradient bar. The issues appeared to be the following:
sizemiddleware in the Popover reduces the size of the popover by looking at the amount of space between the anchor and the edge of the viewport — this is likely not what we want for the horizontal axis, as it reduces the Popover when the Popover is right at the edge of the screen, and also appears to be the root cause of one of the issues in Tooltip: Fix positioning by anchoring to child element #41268 (Tooltip issues).How?
maxWidthsetting from thesizemiddleware used in the PopoverTesting Instructions
CustomGradientPicker:ColorPickerpopover is glitchy (size and position) #41353 and check that the popover displays correctly.Note: clicking on the control points in the gradient bar is a little glitchy at the moment, too — I think some adjustments need to be made there, too, but that's outside the scope of this PR.
Screenshots or screencast