-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CustomSelectControlV2: fix popover styles #62821
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
523ba41
to
540a0ee
Compare
Flaky tests detected in 540a0ee. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9680963724
|
/* TODO: is there a way to read the sass variable? */ | ||
z-index: 1000000; | ||
|
||
max-height: min( var( --popover-available-height, 400px ), 400px ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine for legacy (and for the purposes of this PR), but I think we will also need a way for consumers to override this (#49110).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point — not necessarily part of the tweaks to V2 legacy, but important to keep in mind for future work. It's also tracked in #55023
540a0ee
to
65ddf48
Compare
What?
Extracted from #62424
Fix popover styles of
CustomSelectControlV2
componentWhy?
How?
max-height
andoverflow
styles (in addition to the--popover-available-height
Ariakit CSS variable) to make sure that the popover doesn't get too tallz-index
matching the V1 component (and the legacyPopover
)display: flex
(it feels more appropriate for the layout we want to implement, and it handles better extra white space)Testing Instructions
In the editor
To test in the editor, apply this patch
Click to expand
height
+overflow
fixeesz-index
fixKnown issues
The V2 version of the component automatically "flips" the popover to render on top of the trigger if there isn't enough space below it. In the specific example of the block toolbar, this causes a small visual glitch when the browser's viewport is short enough to cause the flip:
This glitch happens because the block toolbar's header has
position: sticky
, which causes it to render on top of the select popover despite a lowerz-index
. I haven't looked too much into it, but this feels like an issue that shouldn't be solved at theCustomSelectControl
level (should we leverage popover slots?)