Update conditions to hide duotone panel #33295
Conversation
|
Size Change: +69 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
oandregal
requested review from
ajlende,
youknowriad,
jorgefilipecosta and
aristath
oandregal
added
[Feature] Design Tools
oandregal
requested review from
ajitbohra,
chrisvanpatten and
ellatrix
as code owners
There was a problem hiding this comment.
it no longer hides the whole duotone component if the
color.duotoneisnull
I think we should be able to disable duotone without disabling the color palette or custom colors which is why it worked the way it did previously. I'm okay with having null (or maybe false instead?) disable the panel entirely while an empty array just removes the presets.
EDIT: However, it still makes sense to me to hide the whole thing when all three (color.custom, color.palette, and color.duotone) would result in the empty panel shown in #33280 (comment).
I'm sorry I've come late to the discussion, this is a bit disrupting for everyone. Let me share my mindset, which is to aim for consistency among all the tools consumers have and don't optimize for a specific one. The duotone panel can work without duotone presets ( I understand how useful it can be to have a switch to disable panels entirely, be it duotone, colors, typography, etc. What are our options here?
I'd like to hear thoughts from others @jorgefilipecosta @youknowriad @mtias @jasmussen |
oandregal
force-pushed
the
update/hide-duotone-panel
branch
from
9bcb4cf
to
f9c2f67
Compare
There was a problem hiding this comment.
Hi @nosolosw,
I think duotone should be consistent with the other presets we have. In that case we would have duotone presets that are always an array, and a customDuotone flag. Exactly as it happens on color, gradients, and font sizes.
Duotone appearing or not should not depend on colors. Duotone would not appear if duotone presets is empty and customDuotone is false, and it would appear otherwise.
If colors are disabled (color presets and customColors) but customDuotone is true it would still be possible to pick custom colors for a custom duotone. The same happens on gradients if customColors is false but customGradients is true I can still pick colors for building my gradients. They are independent controls.
|
So, Jorge is arguing that, in addition to all this PR does, the whole section below should be able to be disabled via a new
|
|
Exactly, And that part would not be available if color.duotone is an empty array. culor.customDuotone would control this part: And that part would not be available if color.customDuotone is false. If color.duotone is empty and color.customDuotone is false, the whole duotone functionality would not be available at all. And we would not have any reference to duotone on the UI. |
|
@nosolosw I like the idea of having an option to disable panels that is separate from the preset arrays. I wanted to do something like that for duotone when I was working on it, but ended up going with the established pattern instead to avoid confusion for folks. Seems like something worth pursuing for 5.9. Adding a separate
|
|
@ajlende nice, I just did that. I think we all three are in agreement. Now, we need a last little thing: what should happen when
However, I wonder why that bar is controlled by the |
|
My thinking was that the duotone bar should be controlled by So the bar should be controlled by both |
|
Ok, I tried to change the logic locally to see how this feels. This is with
And this is with
|
Oh, wait. Can I interact with the bar to modify those colors? I think this may be the reason why I'm confused. If I should be able to do that, we do need to use |
|
OK, pushed all that you mentioned. I think it's ready to ship. In talking with Jorge about this he raised the concern of using At the same time it has been a long day for me, so I'd welcome more eyes, including my own rested ones tomorrow :) |
|
Thanks for your work on this, and I'm happy to see you working out the best path forward. With regards to supporting custom colors, theme colors, various palettes and such, I don't thin the following note changes anything, but I wanted to point out that as part of #27331, we'll want a new color swatch picker that groups custom colors and swatches together. This is an older design, but it shows the flow:
The final design should be more like what's shown in #27473 (comment). But the ultimate goal is to not have to duplicate the entire palette for every color property there is, but instead surface those once you click the property first. |
|
Cool, I've addressed all the comments and added docs as well. Would anyone mind giving a final review to this and green check as appropriate? I've looked at #33295 (comment) with fresh eyes and still think the pre-existing logic for |
oandregal
force-pushed
the
update/hide-duotone-panel
branch
from
cc3e4a2
to
6bde447
Compare
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
Stems from this issue #33280
Follows up #32002 and #31764
This updates the conditions to show/hide the duotone panel. See full panel enabled for reference:
color.duotoneis empty ornull(it no longer hides the whole duotone component if thecolor.duotoneis null):color.customDuotoneis false or (color.customis false andcolor.paletteis empty) we have this:color.customis false we have this:color.paletteis empty on null (the control is still visible ifcolor.customis not false):color.duotoneis empty andcolor.customDuotoneis false (and also ifcolor.paletteis empty andcolor.customis false, independently from what thecolor.customDuotonevalue is):