Update: Implement new color palette editor component #35783
Conversation
jorgefilipecosta
requested review from
ajitbohra and
chrisvanpatten
as code owners
jorgefilipecosta
added
the
[Status] In Progress
|
Size Change: -140 B (0%) Total Size: 1.09 MB
ℹ️ View Unchanged
|
jorgefilipecosta
force-pushed
the
update/implement-new-color-palette-editor-component
branch
4 times, most recently
from
368d4e1 to
4ccadd4
Compare
jorgefilipecosta
force-pushed
the
update/implement-new-color-palette-editor-component
branch
from
4ccadd4 to
52a623b
Compare
jorgefilipecosta
added
[Package] Components
|
Thanks for taking a stab at this one! Lots of progress. Here's the core of it: Looks good. There are some details: the plus and ellipsis appear scaled down, and now might be a good time to find a generic way to store the allcaps heading and button style:
The panel doesn't appear to be updated in all places:
And the transparent "checkerboard" style might need a little tweaking. Maybe it's an empty card with just a border, and perhaps a label that says "unset"? It also increases the urgency of landing the ItemGroup/collapsed color swatch change as discussed in #35093, because color panels are now even taller: I wonder if there are smaller stepping stone changes we can make here? |
| line-height: 32px; | ||
| `; | ||
|
|
||
| export const ColorHeading = styled( Heading )` |
There was a problem hiding this comment.
I really appreciate that you've eliminated a ton of component-specific CSS in favor of using components. However there's still custom CSS here in the JS file (even if it's delighfully less). In this case, it looks like we'll be using this subheading style quite a bit across the entire codebase. I'm not sure when the best time is to extract reusable components for this is, vs. speeding ahead — but in my mind the biggest benefit of using these new components is that we can avoid having to write custom CSS. Perhaps a rule of thumb could be: whenever we find a need to write custom CSS to make the components behave, that means the components themselves need to be updated and refined. In doing so, we can elevate the entire interface. What do you think? CC: @mirka
There was a problem hiding this comment.
Exactly, I'll link this back to #35464 for when we do a refactor.
It would be very useful for auditing/refactoring purposes if we could add a story for this ColorEdit component to the Storybook now.
|
I also noticed that with the new edge-to-edge color swatch crops the eyedropper handle, I think we need a way to fix that. There must be some overflow somewhere preventing it from breaking out. |
|
|
||
| export const ColorNameInputControl = styled( InputControl )` | ||
| ${ InputControlContainer } { | ||
| background: #f0f0f0; |
There was a problem hiding this comment.
Let's use COLORS.gray[ 100 ] here. (The closest pattern to this I could find was SearchControl, and that also uses $gray-100.)
| `; | ||
|
|
||
| export const ColorNameContainer = styled.span` | ||
| line-height: 32px; |
There was a problem hiding this comment.
I'm guessing we can simplify some of the height handling. Would it be easier to set a fixed height on ColorItem instead of this line-height and the fixed height in ColorNameInputControl? It might eliminate this jump too:
| background: #f0f0f0; | ||
| border-radius: 2px; | ||
| ${ Input }${ Input }${ Input }${ Input } { | ||
| width: 160px; |
There was a problem hiding this comment.
Do we need this fixed width? It seems like something that should grow to fill the available space.
| .components-button.has-icon { | ||
| min-width: 0; | ||
| padding: 0 2px; |
There was a problem hiding this comment.
This doesn't seem to be working well for the "Remove color" button:
|
|
||
| export const DoneButton = styled( Button )` | ||
| && { | ||
| color: #3858e9; |
There was a problem hiding this comment.
We can't hardcode these values because the wp-admin theme color can vary by user. Let's use COLORS.ui.theme here.
| line-height: 32px; | ||
| `; | ||
|
|
||
| export const ColorHeading = styled( Heading )` |
There was a problem hiding this comment.
Exactly, I'll link this back to #35464 for when we do a refactor.
It would be very useful for auditing/refactoring purposes if we could add a story for this ColorEdit component to the Storybook now.
jorgefilipecosta
force-pushed
the
update/implement-new-color-palette-editor-component
branch
3 times, most recently
from
7ffbb2c to
d1b4496
Compare
|
Hi @jasmussen,
This PR changes the color palette editor. The palette editor is only available on the global styles sidebar color palette section. The palette editor allows the user to change the colors that are available on the user palette. When picking a color, the color palette editor is not available. We show the color one can pick but don't allow editing as there is an area for that. These places are the color picker and not the palette editor.
Good suggestions. I will try to find some time to propose a PR with these changes as this affects the color picking mechanism but not the color palette editor that this PR is focusing on.
Yes, we should try to land this PR as soon as possible. Hi @mirka, Thank you for the review. I tried to apply all the suggestions! |
|
Hey, small quick check: in the card to open the palette screen (color edit component), what are the colors that should be shown there? I presume only user colors? I've reset the user styles so I start with no user colors and this is what I've experienced. Using tt1-blocks I see the theme colors listed there:
Using empty theme I see no color listed (empty theme has
Using empty theme I see core colors if I remove the
|
|
Are users able to have different color palettes per block? Themes can do this via: I've tested this flow as a user and this is what I see (the same single color palette everywhere): color-palettes.mp4(Again, sorry for the too many questions you may have already responded to in other places. I'm lacking tons of context here). |
This seems to be an already existing bug on the panels. Instead of opening a panel specific to edit user colors we go to the global one. This can be seen by the way we go back (instead of going back to the block we go back to the global). I will try to address this navigation issue in another PR. |
|
Got a fix for the navigation issue at #36203 |
jorgefilipecosta
force-pushed
the
update/implement-new-color-palette-editor-component
branch
from
d1b4496 to
c0a7a21
Compare
|
Hi @oandregal, after the fix #36203 user colors seem to work well on this PR. |
There was a problem hiding this comment.
This works as advertised.
We can address #35783 (comment) as a follow-up.
Haven't done a deep dive into the code but it generally makes sense. We can address changes post-merge as well.
jorgefilipecosta
deleted the
update/implement-new-color-palette-editor-component
branch
This PR implements the new custom color palette editor design:
It also changes the logic of how things work previously the color palette editor worked on the merged colors, e.g: the default was the theme colors. Now the plan is to show core, theme, and custom colors, so the custom color palette editor works on user's colors that by default are empty.
In the future, we will show the three palettes but in order to keep this PR small, it only operates on the custom color one. The others are not editable anyway and will only be shown.
@pablohoneyhoney, @jasmussen would you be able to review this PR from the design and UX point of view?