Global styles: add onChange actions to color palette items #45681
Conversation
ramonjd
added
[Type] Enhancement
|
|
|
Size Change: +162 B (0%) Total Size: 1.3 MB
ℹ️ View Unchanged
|
|
If I can edit the theme colors when the list is closed, then what is the purpose of having the ability to "edit" the colors anymore? |
Good question, something I didn't think of. This is probably the advantage of @pbking's approach in #45657 - removing it completely. I guess there's only the question of whether we need the "Edit" functionality for either PR 😄 |
|
Another idea to improve this PR from @scruffian and @SaxonF was to change the label of "Edit" to "List view" indicate that there's an alternative view (given that this PR allows direct editing of colors) |
|
The benefit of the "edit" mode is that it shows a list of the items with names, so it is useful to be able to switch between states, but the label should be "List view" or similar, not edit. |
|
A couple of observations:
|
Good idea. This would be better, I agree. I've done some hacking and it looks possible, but it requires a bit of a refactor and will bloat this PR a bit. I can take it as far as I can and we can assess.
Hmm... If we allow edit in place then the list view becomes redundant, aside from showing the palette item names. Maybe "Show color/gradient labels?" That's a bit wordy. "Show more", "Show details", "Where is my thesaurus?" |
I've taken a stab at this in 581ea0b It's messy, but it seems to work 😄 |
…or or gradient by clicking on it.
…e list view. Rename "List view" to "Show details"
…adient with the same incoming value, then just open up the list view editing panel.
ramonjd
force-pushed
the
update/color-palette-global-styles-actions
branch
from
8922ea1 to
051b23f
Compare
|
It works! Nice job. It definitely feels nicer now that something happens when you click on a colour circle in Palette UI. It felt really broken before. I do wonder if this makes changing an existing colour too easy. It's not something I expect users to do often. (I could be wrong?) It also greatly increases the likelihood that a colour is named incorrectly. For example, it's now really easy to set the "Pale pink" colour to be blue without renaming "Pale pink" to "Pale blue" or whatever. Part of me wonders if we should just disable the button altogether so that it's a completely non-interactive colour circle until you enter edit mode. What do you think @jasmussen? |
|
Thanks for taking a look at this @noisysocks
Good point. This would apply only to custom palettes though, right? That is, colors a user has already actively added. Maybe it's not as invasive (?) Also, the commit history of this PR (see comment) has a version that opens the edit "list view" when clicking on a color, so maybe there's a compromise there.
Another valid option 😄 It's the apparent "clickability" that's causing confusion for me personally, so making it obvious that the palette options aren't to be clicked might also work. |
When I try this PR out locally you can edit all palettes not just the custom one. If we limited it to the custom palette then that would definitely alleviate my concern. |
Sorry, my bad. My brain was thinking "changing the text label" and not the color. Withdraw my comment 😄 |
I would tend to agree, right now it feels really weird that I can select a spot color but then, nothing. As a small improvement, this seems like a step in the right direction. But I do wonder: if the only remaining thing that the edit flow allows, is to rename colors, why not just show that? It's not a strong opinion, and by still gating it, it might make for a simpler experience in 80% of cases. CC: @mtias |
I think renaming colors only applies for custom colors (?), so theme and default colors in edit mode allow editing colors. The simplicity of #45657, which shows the editable list view all the time, appeals to me personally. I think the only concern raised what that it has the potential to take up a fair thwack of real estate, and, as a consequence, remove the opportunity to add more items to the sidebar in that specific view. As an example, here what I'm seeing in 2023 with a few custom colors:
The browser viewport height is 1100px |
|
Yeah, way better than what's in |
|
👋 Hey there! Would it be possible to bolster unit tests to |
I always forget the changelog! Thanks for the reminder 👍 |
- Add changelog for changes merged in #45681 - Test case for Popover
- Add changelog for changes merged in #45681 - Test case for Popover
- Add changelog for changes merged in #45681 - Test case for Popover
* Initial commit: - Add changelog for changes merged in #45681 - Test case for Popover * Change log entry * Formatting * Changelog formatting Relying on existing control aria labels to test for the presence of gradient and color pickers rather than the container. * Remove unnecessary async function dec * Export no longer required * Testing async queries using findByLabelText because the CI tests are failing (not local) * Let's try this. * Let's try this. * What about this? * Why not * And so it continues * Removing tests so we can debug them separately * Reinstate test for color only. Gradient needs some async magic
* Initial commit: - Add changelog for changes merged in WordPress#45681 - Test case for Popover * Change log entry * Formatting * Changelog formatting Relying on existing control aria labels to test for the presence of gradient and color pickers rather than the container. * Remove unnecessary async function dec * Export no longer required * Testing async queries using findByLabelText because the CI tests are failing (not local) * Let's try this. * Let's try this. * What about this? * Why not * And so it continues * Removing tests so we can debug them separately * Reinstate test for color only. Gradient needs some async magic
What?
Resolves #44138
The PR opens the color picker when palette item is selected.
Renames the "Edit colors/gradients" label with "Show details" (suggestions for alternatives welcome).
How?
By inserting a new instance of the color picker popover.
We track the currently-edited color/gradient by its index in the colors/gradients collection. The collection is rendered to the page according to this index.
We need the index of the currently-edited color/gradient to ensure we're editing the correct item in the collection.
We can't check by value or slug because two or more items might share these values. This is why this PR also updates the color/gradient picker component to return the index of the item to the onChange callback. Using this value we can lookup the currently-edited item in the colors/gradients collection.
As a fallback, if index doesn't match an item OR the matched item's value does not equal the newly-chosen value, we just open the list view as before.
Why?
Before nothing happened when you clicked on a color or gradient option. Weird!
Testing Instructions
The color picker should show for the color you clicked on, and you should be able to adjust the color successfully.
Test it out with gradients too!
Screenshots or screencast
2022-11-17.15.37.37.mp4