Change z-index override logic for popovers.#17696
Conversation
tellthemachines
requested review from
ajitbohra,
chrisvanpatten,
ellatrix,
gziolo,
jorgefilipecosta,
talldan and
youknowriad
as code owners
youknowriad
added
the
[Type] Bug
jasmussen
left a comment
jasmussen
left a comment
There was a problem hiding this comment.
In my testing, this works well, thank you for the PR! Testing steps pass:
I like the simplified code, and in testing various other popovers, things seemed good to me.
I left one comment about a gradient syntax change that seems unrelated to the PR, but other than that, this works for me.
| theme(button) 72% | ||
| ); | ||
| background-image: | ||
| linear-gradient(-45deg, theme(button) 28%, color(theme(button) shade(20%)) 28%, color(theme(button) shade(20%)) 72%, theme(button) 72%); |
There was a problem hiding this comment.
Any reason for this change other than personal preference?
Gradients always give me headaches, and although it's not really a coding pattern to split on to multiple lines, when actually working on these gradients and tweaking them, it really helps that they're on multiple lines. I'd prefer the old version unless there's a good reason to change this.
There was a problem hiding this comment.
Oooh interesting. Stylelint changed that automatically so I guess the /* stylelint-disable */ comment above is being ignored for some reason. I'll revert that as it's unrelated to this changset 😛
There was a problem hiding this comment.
Done, thanks for the review @jasmussen !
tellthemachines
force-pushed
the
fix/popover-zindex
branch
from
383d3ee to
6e48ccf
Compare
tellthemachines
requested review from
SergioEstevao,
daniloercoli and
etoledom
as code owners
* Change z-index override logic for popovers. * Undo gradient formatting. * Remove import of deleted file. * Remove empty file, fix reference to deleted zindex
3 participants
Description
New popovers in the sidebar won't display on mobile because of their z-index. This fixes that issue.
Found this issue while working on #17418 - turns out that
:not(.is-mobile)actually targets all mobile instances of Dropdown/Popover unless they are also set toexpandOnMobile.The rule I'm removing was originally added to stop popovers at the top of the page from hiding the wp-admin bar dropdowns, but it is too general so over time a bunch of overrides for it have been added.
My change removes that rule and the overrides, and instead adds specific rules to keep the z-index of the top-of-the-page popovers under wp-admin dropdowns.
Steps to test
Open editor on desktop. Click to open block inserter, table of contents, or block navigation in the top toolbar, then hover over "+ New" on wp-admin bar so that the dropdown appears. Verify that the wp-admin dropdown is displaying over the popover from the editor toolbar.
Open editor on mobile. Select a text block and open settings bar for the block. Go to "Color settings"; clicking on "custom color" should bring up a popover.
How has this been tested?
Checked all the popovers I could find in the editor, on desktop and (emulated) mobile. Triple-checked the ones I added or removed specific rules for.
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: