Merged
Conversation
gwwar
reviewed
May 19, 2021
|
|
||
| const duotonePalette = useSetting( 'color.duotone' ); | ||
| const colorPalette = useSetting( 'color.palette' ); | ||
| const disableCustomColors = ! useSetting( 'color.custom' ); |
Contributor
There was a problem hiding this comment.
Could we inverse this? It reads more nicely without the double negative:
const customColors = useSetting( 'color.custom' ); // or enableCustomColors
///...
{ customColors && (
<CustomDuotoneBar value={ value } onChange={ onChange } />
}
Contributor
Author
There was a problem hiding this comment.
I was keeping things consistent with how PanelColorGradientSettings works, but keeping that consistency probably doesn't matter that much
Contributor
There was a problem hiding this comment.
Oh interesting, this one does personally read a bit awkwardly, but It's non-blocking if we're intentionally following this pattern.
Contributor
|
Reserving this comment for manual testing: Image to Cover TransformIt looks like this will throw an error when duotone is not set on an image. We can reproduce this with any combination, by clicking on "add text to image" button before duotone is applied.
With duotone disabled✅ No duotone options appear on toolbar Duotone enabled, No custom colors/pallets✅ UI looks correct on toolbar
Duotone enabled + custom colors✅ UI looks correct on toolbar
Duotone enabled + palette✅ UI looks correct on toolbar This is best left for a follow up PR, but I also noticed that the first time we set a color value, this dismisses the duotone dropdown: options.mp4Everything enabled
|
Contributor
Author
|
Transform from image to cover seems to have that same error even when duotone isn't disabled. I'm looking into a fix that I'll open up in another PR EDIT: added #32006 |
7 tasks
|
Size Change: +67 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
Contributor
|
The test failure is already on trunk and is being investigated, so I'm merging this one. |
Contributor
|
❤️ |
Member
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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
Fixes #31764
This allows for duotone to be disabled when
color.duotoneis set tonullin theme.json as well as respecting settings whencolor.custom === falseandcolor.palette === null.How has this been tested?
Change settings in theme.json as shown below in the screenshots section.
Screenshots
{ "version": 1, "settings": { "color": { "custom": true, "duotone": [], "palette": [] } } }{ "version": 1, "settings": { "color": { "custom": false, "duotone": [], "palette": [] } } }{ "version": 1, "settings": { "color": { "custom": true, "duotone": [], "palette": null } } }{ "version": 1, "settings": { "color": { "custom": false, "duotone": [], "palette": null } } }{ "version": 1, "settings": { "color": { "duotone": null } } }Types of changes
New feature / Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).