Border panel: Update to display multiple palette origins #36753
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
stacimc
added
[Type] Bug
|
Size Change: +15 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
7 tasks
stacimc
force-pushed
the
update/border-color-to-display-multiple-palette-origins
branch
from
ee476df to
163bddb
Compare
There was a problem hiding this comment.
Thanks for putting this together @stacimc 👍
We definitely need it to make sure the border-colour control is brought up to date for 5.9.
Alternative to #33743
For others coming to this PR, it is more "related to" or a subset, rather than a full alternative to #33743.
I've given this a quick test and it looks to be working for me. I did leave one tiny comment though that I'll leave up to you.
It would be good to get a second set of eyes on this to see if there is a better approach. Given my input in creating these changes a final tick of approval should probably be provided by someone else.
| onColorChange={ onChangeColor } | ||
| clearable={ false } |
There was a problem hiding this comment.
I believe this clearable prop was only added to #33743 because the control could be reset via the ToolsPanel menu. Given this PR is for before that lands perhaps it should be omitted here.
It's only a small thing. I don't feel strongly either way.
There was a problem hiding this comment.
I don't feel strongly about it either. For consistency with the color panel, it'd make sense to omit it? In manual testing, you can select a theme or default color and then deselect it by clicking the circle to effectively clear the color setting, so I don't think it's a big issue if we leave it in, though.
|
I've been testing this and have noticed no real problems in the border panel color palette controls. They behave and look like the color panel controls.
Are there any preferred testing steps? Those PRs are very light on testing details, the latter contains none at all. Also, I tried testing with a custom color palette in the Site Editor –
– but it never saves. This is also happening on trunk so... 🤷 |
aaronrobertshaw
force-pushed
the
update/border-color-to-display-multiple-palette-origins
branch
from
163bddb to
e6afdeb
Compare
I've tested this as well but it is working for me on trunk. It looks like it was fixed yesterday when #36817 was merged. Prior to rebasing this PR I could replicate the issue, after rebasing, it is resolved.
|
andrewserong
approved these changes
Nov 29, 2021
There was a problem hiding this comment.
This is testing nicely for me, with default and theme colors. It also still works well in a theme.json that doesn't provide palette colors and also switches off the default palette by setting settings.color.defaultPalette to false — here it's still possible to set a custom border color:
LGTM, and the regular color palettes still appear to be behaving correctly, and I like the refactor of adding in the useMultipleOriginColorsAndGradients hook — looks like it was safely moved over without changing anything 👍
| onColorChange={ onChangeColor } | ||
| clearable={ false } |
There was a problem hiding this comment.
I don't feel strongly about it either. For consistency with the color panel, it'd make sense to omit it? In manual testing, you can select a theme or default color and then deselect it by clicking the circle to effectively clear the color setting, so I don't think it's a big issue if we leave it in, though.
aaronrobertshaw
deleted the
update/border-color-to-display-multiple-palette-origins
branch
|
Does this need to be backported to WordPress 5.9? |
@oandregal Just added it to the Must-Haves project; I moved it directly to |
aaronrobertshaw
added
the
Backport to WP 6.7 Beta/RC
To fix #36641 for 5.9 so all the color controls display the multiple origin palettes I believe this should be backported. I've added the |
noisysocks
pushed a commit
that referenced
this pull request
Nov 30, 2021
* Extract color and gradient settings retrieval to custom hook * Update border color to display multiple color palette origins Co-authored-by: Aaron Robertshaw <[email protected]>
noisysocks
removed
the
Backport to WP 6.7 Beta/RC
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 #36641
Alternative to #33743Description
The color panel is able to show three different palettes: the theme, the default, and the user one. This updates the border panel to provide the same options for border color.
@aaronrobertshaw implemented this fix in #33743 on top of work to switch the Border panel to use the ToolsPanel. That PR is currently blocked on a discussion about how to handle defaults and base styles. The purpose of this PR is to have an alternative to fix the multiple palette issue in 5.9 just in case #33743 is not merged.This fix was pulled out of #33743, which switches the Border panel to use the ToolsPanel. It is pulled out into its own PR for ease of getting it into 5.9, since it fixes #36641.
How has this been tested?
Color and gradient settings retrieval are extracted without change into a hook, but we should still retest the Colors panel using instructions from #35970 and #36492.
Test border color from all origins and verify that the behavior in the border panel matches the Colors panel.
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).