Fix: Custom color picker popover position #36965
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jorgefilipecosta
added
Backport to WP 6.7 Beta/RC
jorgefilipecosta
requested review from
ajitbohra,
ellatrix,
mkaz and
tellthemachines
as code owners
|
Size Change: +213 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
jorgefilipecosta
force-pushed
the
fix/custom-color-picker-popover-position
branch
from
b02675a to
d39d1a0
Compare
oandregal
approved these changes
Nov 29, 2021
ntsekouras
reviewed
Nov 30, 2021
| @@ -135,7 +137,12 @@ export default function ColorPalette( { | |||
| <VStack spacing={ 3 } className={ className }> | |||
| { ! disableCustomColors && ( | |||
| <Dropdown | |||
| contentClassName="components-color-palette__custom-color-dropdown-content" | |||
| contentClassName={ classnames( | |||
There was a problem hiding this comment.
Could we just pass down a css class instead of introducing a new API?
There was a problem hiding this comment.
We would need add a property like customColorContentClassName, to ColorPalette, and ColorGradientPanels etc. A block wanting to have the color picker appear in the correct position would need to explicitly know the class to add.
It seems preferable to have a flag that changes the positioning behavior. The fact we use a class to do that is an implementation detail, e.g: on mobile, the way to do it may be different.
ntsekouras
reviewed
Nov 30, 2021
| @@ -25,3 +25,10 @@ | |||
| padding: 0; | |||
| } | |||
| } | |||
|
|
|||
| @include break-medium() { | |||
| .components-dropdown__content.components-color-palette__custom-color-dropdown-content.is-rendered-in-sidebar.is-from-top .components-popover__content { | |||
There was a problem hiding this comment.
This is really coupled with internal class names of other components. Do you think we could avoid them or extract them somehow?
There was a problem hiding this comment.
Here we are changing the position of a popover, I'm not sure how we can avoid relying on the classes the component defines. And unfortunately, this is not the first time we do it, searching for ".components-popover__content" on the codebase we find lots of matches used on each place we change something related to the popover, in some cases even outside the components package while on this case we are inside the components package.
It is not an ideal solution but it seems to be consistent with what we are doing on similar cases.
jorgefilipecosta
deleted the
fix/custom-color-picker-popover-position
branch
|
Nice work! When we have the chance, it's worth aligning the corners of the flyout to have that slight consistent chape.
Like this:
|
youknowriad
reviewed
Dec 2, 2021
noisysocks
removed
the
Backport to WP 6.7 Beta/RC
noisysocks
pushed a commit
that referenced
this pull request
Dec 6, 2021
57 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #36542
Currently, the custom color picker popover appears over the sidebar on the block inspector and on the global styles.
This PR fixes the issue and makes the custom color picker appear on the side as shown on the screenshot below.
In order to fix the issue, we are introducing an experimental __experimentalCorrectPositionForSidebar flag, that when true applies a special positioning for the popover. We are adding an option flag and not using this behavior by default because the component may be used outside of a sidebar and in that case that behavior should not apply.
How has this been tested?
I verified that in all the following cases the custom color picker appears at the side of the sidebar:
Screenshots