Palette Edit: Don't discards colors with default name and slug

[t-hamano]

Fixes: #37861
Related to #37496

What?

This PR will not discard colors with the default name (Color-{number}) and default color (#000) in the PaletteEdit component.

Why?

I have looked at the changes made in #37496 and this PR makes two main changes:

• Apply the default slug when adding a new color.
• If a color has a default slug and a default color, discard that color as temporary

However, my feeling is that the newly added color will have an appropriate name and slug so that the color no longer needs to be considered a "temporary" color.

In addition, you can never save a color that is considered a temporary color (i.e., one whose name is in the form color {number} and whose HEX color is #000). To me, this behavior feels more like a bug.

How?

I removed the code related to the isTemporaryElement() function that determines if a color is temporary or not, so that colors with default name are not deleted. I also adjusted variable names and comments.

Note: The PaletteEdit component generates a slug from the input string, but problems exist regarding non-Latin and multibyte characters. See #49711 and #39210 for details.

Testing Instructions

• Access the Global Style panel.
• Press the "Add" button on the Custom Color palette in succession.
• Colors named Color 1, Color 2, and Color 3 should be created.
• Click the "Done" button.
• The color palette should be retained.

Screenshots or screencast

34561cfcc445476fa0c4bb874199df15.mp4

[github-actions]

Flaky tests detected in 676611a.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7296048303
📝 Reported issues:

• [Flaky Test] Should show Columns block only if selected blocks are between limits (1-6) #40369 in specs/editor/various/block-switcher.test.js

[ciampo]

The changes proposed in this PR make sense to me, although I'd like to hear thoughts from other folks too.

---

I also wanted to flag a related issue, which is going to be happening more often if this PR gets merged: entries with the same color name and/or value all appear as selected at the same time:

• ColorPalette: make component more resilient to color entries with same color value and/or name #43197
• Global Styles - Custom Colors given the same name cannot be applied properly #49593

It would be great if we could look into fixing those issues as a follow-up

[t-hamano]

When I merged the latest trunk into this PR and resolved the conflict, I discovered #56896 regarding the gradient custom palette.
This PR will also no longer discard gradients with the default color and the default color name.

The screencast below shows how to apply the default gradient color, but we discovered that a warning error is logged in the browser. However, this error also appears to occur on trunk, so I think it needs to be fixed separately.

922efb48fcabfe2594b7994698e47c2a.mp4

[ciampo]

Adding @ramonjd @tellthemachines and @andrewserong as reviewers as they've been also working on PaletteEdit in the past weeks and are currently working on #56932

[ramonjd]

Oh wow, thanks for pointing out this PR to me @ciampo

I'll test it out tomorrow. Hopefully we can close #56932

[ramonjd]

I'm strangely attracted to this approach because:

1. It removes a complexity
2. It removes a lot of code anyway
3. It returns us to a state from which we might try alternatives
4. It hands more power and choice to the user

[ramonjd]

I actually think this is a decent approach to removing the complexity. Why do we remove "default" values? To keep the list clean? We could rather trust the user to control how they want to create custom colors/gradients.

The following doesn't make much sense, but why not?

2023-12-21.11.02.16.mp4

The only thing we could probably address here is the lack of default value on the first item of the gradient:

It has an empty value and can be saved.

[andrewserong]

The following doesn't make much sense, but why not?

That's a good point... maybe the user knows they're going to add a few custom colors and quickly goes to create them and then decides to go back later to update their colors? It might be nice to allow these sorts of interactions rather than try to predict too much, so I'm really liking the direction of this PR.

[andrewserong]

I'm strangely attracted to this approach because:

+1, I really like the approach here! I think going for simplicity where we can is a good approach, and in practice, it feels quite good to me that you hit "+" and then new things are created and its up to the user to delete the ones they don't want.

The only issue I ran into is with a bit of inconsistency in creating gradients, not sure if it's related to what Ramon observed about an empty value being able to be saved, but when I go to add the first gradient, it has an empty value, but the second one isn't empty:

2023-12-21.13.49.22.mp4

[t-hamano]

Thanks forthetesting, @ramonjd, @andrewserong!

The only issue I ran into is with a bit of inconsistency in creating gradients, not sure if it's related to what Ramon observed about an empty value being able to be saved, but when I go to add the first gradient, it has an empty value, but the second one isn't empty:

Thanks for finding that problem! This also occurs with trunk, so it should be a problem that needs to be fixed separately.

The cause is that if the custom gradient palette does not yet exist, this condition will be false, so the first palette added will be recognized as a solid color.

d7aca5dc537dcf27116c8e267dad9fe9.mp4

[t-hamano]

The cause is that if the custom gradient palette does not yet exist, this condition will be false, so the first palette added will be recognized as a solid color.

I'm starting to think it might be better to resolve the above issues first before tackling this PR.

Whether the PaletteEdit component uses a solid color picker or a gradient color picker is determined solely by whether the gradients prop is falsy, so if no palette exists yet, there appears to be no way to make the PaletteEdit component act as a gradient color picker.

For example, add props like isGradient to specify the color type?

@ciampo Please let me know if you have any ideas 🙏

[andrewserong]

I'm starting to think it might be better to resolve the above issues first before tackling this PR.

I'd be fine with either personally, since as you mention that's happening on trunk. Let's see what Marco thinks, but if it makes it easier to land this PR first before looking at that other issue, that would sound reasonable to me!

[ciampo]

I agree with Andrew, let's resolve any feedback left and merge this PR, as it is an improvement over trunk anyway.

To recap, here are some follow-ups:

1. Improve consistency when adding new gradients (issue described here)
   • While doing so, we should also investigate the console error described here, which seems related.
2. Improve how we handle entries with the same name / color value — this is a pre-existing issue, but I think it's going to be amplified as a consequence of the changes in this PR
   • ColorPalette: make component more resilient to color entries with same color value and/or name #43197
   • Global Styles - Custom Colors given the same name cannot be applied properly #49593
3. Issues when handling non-latin characters (as mentioned in the PR description)

Unfortunately, I don't currently have time to work on this — @t-hamano / @andrewserong / @ramonjd , would any of you be able to look into it?

[t-hamano]

Unfortunately, I don't currently have time to work on this — @t-hamano / @andrewserong / @ramonjd , would any of you be able to look into it?

OK, I have summarized the issues to be addressed in #57309.

[andrewserong]

Sounds like we all agree, let's merge! ✨

(Looks like there's just a changelog conflict to resolve 🙂)

[ramonjd]

Great stuff! 🚢