Global Styles: Fix display of color palettes for border panel #38798
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
aaronrobertshaw
added
[Type] Bug
|
Size Change: +4 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
noisysocks
reviewed
Feb 28, 2022
There was a problem hiding this comment.
I followed your testing steps and it fixes the bug! 👍
The code looks really good to me, too.
Just a question on whether to refactor useMultiOriginColors a little bit.
Also, what's the state of E2E testing for global styles? Is it worth adding a regression test for this bug?
| @@ -68,6 +65,52 @@ function useHasBorderWidthControl( name ) { | |||
| ); | |||
| } | |||
|
|
|||
| function useMultiOriginColors( name ) { | |||
There was a problem hiding this comment.
There's some overlap between this hook and useMultipleOriginColorsAndGradients in packages/block-editor/src/components/colors-gradients/use-multiple-origin-colors-and-gradients.js. Do you think we should DRY that up? Or do you think it's too soon?
I can envision that a future developer might fix a bug or update a string in useMultipleOriginColorsAndGradients but not useMultiOriginColors because the Border panel that appears in the block settings sidebar is more obvious than the one that appears in global styles.
There was a problem hiding this comment.
There's some overlap between this hook and useMultipleOriginColorsAndGradients
Thanks for the nudge to DRY this up.
Upon revisiting this I've found the useColorsPerOrigin hook that was introduced to the site editor not too long before I opened this PR. I missed it as I was expecting a similar name to what was already in the block editor.
It took the same approach as the hook I previously had in this PR so I've updated these changes to leverage that instead. This means that all the color supports (background, link & text) along with border color use the same hook now.
aaronrobertshaw
force-pushed
the
fix/global-styles-border-color-palettes
branch
from
bacfeab
to
c8da70d
Compare
|
Thanks for the review @noisysocks 🙇
I'm not clear on what sort of coverage we are aiming for with global styles. Now, this PR leverages the existing There are also a few open PRs ( #37769, #37770, #38876 ) that should be close to landing that will re-work the border controls. As such, I'd be tempted to delay adding any testing around the border UI in Global Styles until they land. Ultimately, I'm not sure if it is worth adding a regression test for this but I don't feel strongly about it. |
glendaviesnz
approved these changes
Mar 17, 2022
|
I think a rebase will fix the test failures. |
Also switches to use the new ColorGradientSettingsDropdown like the block editor.
aaronrobertshaw
force-pushed
the
fix/global-styles-border-color-palettes
branch
from
c8da70d
to
ceb6646
Compare
jostnes
pushed a commit
to jostnes/gutenberg
that referenced
this pull request
Mar 23, 2022
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.
Description
The Global Styles border panel appears to only display the last source color palette. This PR updates the Global Styles border panel to display multi-origin color palettes as well as use the same dropdown style as the block editor for border color.
Testing Instructions
Screenshots
Types of changes
Bug fix.
Checklist:
*.native.jsfiles for terms that need renaming or removal).