Color Controls: Merge color dropdown components #40084
Conversation
aaronrobertshaw
added
[Type] Enhancement
|
Size Change: -134 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for following up with this one @aaronrobertshaw! It took me a little while to wrap my head around the value of the combined component, because it does introduce some additional complexity, where I found the two separate components a little easier to read in isolation.
But, I think the benefits of having a single component as the entry point to rendering the controls makes it worthwhile, and it looks like there's some further consolidation we can do to neaten things up, too, in follow-ups should we wish to do that.
It's also good to see the styles moved over from hooks/color.scss to the colors-gradients directory 👍
This LGTM, but you might want a second opinion from @youknowriad since he suggested the idea (or otherwise @ciampo always has good insights into these things!)
packages/block-editor/src/components/colors-gradients/dropdown.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/colors-gradients/dropdown.js
Outdated
Show resolved
Hide resolved
|
Thanks for the review @andrewserong 👍 I think you capture the tradeoff well in your comment above. Given we appear to be leaning towards this PR's direction I've made the suggested updates to keep this moving. |
|
Thanks @aaronrobertshaw, from a quick skim that's looking good to me, and makes things a bit tidier. Also, on balance, this PR removes more than it adds, which is a win! 👍
|
|
I'd love a quick look from @jorgefilipecosta as he was involved in the initial components here quiet a lot. |
|
Thank you everyone for the feedback and suggestions. I believe we have a consensus to proceed with this refactor. @jorgefilipecosta Do you have any objections with this PR landing? |
There was a problem hiding this comment.
This PR looks good, nice change @aaronrobertshaw 👍 I'm not sure if we need an optional property to allow the component to be used in two versions. I think we can just use the tools panel everywhere, so all the blocks using colors automatically get the new UI.
Currently, the cover, social links, etc, are not using the new UI. These may get solved if we address the previous point. If not we should enable the new flag on these blocks. If we really introduce the flag I think it should be experimental.
| ) ) } | ||
| <ColorGradientSettingsDropdown | ||
| enableAlpha={ enableAlpha } | ||
| isItemGroup={ false } |
There was a problem hiding this comment.
Do we really need the isItemGroup prop can we just use the new UI by default everywhere? Recently we changed the component to show the palettes in a popover everywhere without a prop and that was an even a bigger change. I guess we can just use tools panels everywhere.
|
Thanks for taking the time to look at this @jorgefilipecosta 👍 Apologies in advance for the long reply.
I'm not sure whether we can just use the ToolsPanel everywhere. The Using the tools panel everywhere would also likely entail removing or updating the Furthermore, I can still see some blocks wishing to add arbitrary color controls in a semantic way outside of a
We can, as you suggest, update the Cover and Social Links blocks to inject their color-related controls into the color block supports
The difference I see here compared to updating the component to show the palette in a popover is, at that point in time there was only one type of panel the color controls were used in so they all had the same constraints. Nothing really changed for their consumers. The conversion of the colors block support to use a tools panel was pending review and approval when the popover updates to the color controls were made. I was under the impression there was a use case for rendering them in an I don't feel strongly about which way we go, I'd like to clean up the code here and reduce the complexity as much as possible while we are at it. So that said, some options to move forward could be to:
Thanks again for your help and guidance on this one @jorgefilipecosta 🙇 |
There was a problem hiding this comment.
Thank you for the detailed explanation @aaronrobertshaw.
I think this PR is a step in the good direction of unifying color components. For now, I think it would be better to have isItemGroup as experimental so that in the future, we are able to remove it.
In the future, I think it would be good to try to make PanelColorGradientSettings and PanelColorSettings use the tools panel, but we can try that in different PR's as we are not sure if it fits or not. I think we can keep the same props the components have. And by default, if we provide this experience in every block it is a good idea, on the cover the default may not be optimal but it is not too bad either following the screen @aaronrobertshaw shared.
Regarding adding additional props to PanelColorGradientSettings to make it fit a tools panel I don't think we need that the PanelColorGradientSettings already can reset and manipulate the colors it would be a matter of inside it adapting the properties so a tools panel could be used.
These changes update the ColorGradientSettingsDropdown to render differently depending upon whether it will be representing a fixed semantic `ItemGroup` or a collection of color controls within a `ToolsPanel` e.g. for the color block supports panel.
aaronrobertshaw
force-pushed
the
try/merging-color-dropdown-controls
branch
from
4efe4b4 to
025392f
Compare
|
Thanks for giving this another look @jorgefilipecosta 🙇
✅ Done. I've made
I agree it would be nice to unify everything here to use the
When I mentioned the These are required for the panel to behave as per the others. The color control itself might allow for a value to be cleared but it will still be represented within the We could easily do all this for our uses of the For now, I believe we have a consensus to land this PR and explore the rest separately. I'll aim to merge this tomorrow pending any last minute objections. |
|
Yes, the consensus is to merge this PR.
PanelColorSettings may create all the props (hasValue, onDeselect, and resetAllFilter) based just on the color values before passing them to the ToolsPanel. So I think we can make PanelColorSettings use ToolsPanel without developers needing to pass any new prop to PanelColorSettings but that's something we can try in the future. |
|
Thanks everyone for the feedback and help getting this one merged. 🙇
I'm a bit at a loss as to how we'd be able to reliably create the If you have some ideas here, I'd be happy to help when the time comes to explore them. |
Related:
What?
In #34027, a new
ToolsPanelColorDropdowncomponent was introduced to facilitate color dropdowns being rendered within theToolsPanelused by the color block supports slot.As a result of further discussion, this PR looks to merge this
ToolsPanelColorDropdownwith the originalColorGradientSettingsDropdown.Why?
Given the overlap, refactoring these to a single component, should reduce maintenance, highlight how color controls may need to be rendered differently in different contexts, and help ensure both cases are updated appropriately should the color control need changing.
How?
ColorGradientSettingsDropdowncomponentItemGroup(original component) or as aToolsPanelItemToolsPanelColorDropdownwith the new mergedColorGradientSettingsDropdowncomponent for the color block supportspackages/block-editor/src/components/colors-gradients/style.scssTesting Instructions
(This tests the
ToolsPanelItemversion of the color dropdown component)ToolsPanelmenu(This tests the original
ItemGroupversion of the color dropdown component)Screenshots or screencast
ColorControls.mp4