PaletteEdit: refactor custom color/gradient name assignment #56932
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ramonjd
commented
Dec 10, 2023
ramonjd
added
[Type] Bug
|
Flaky tests detected in 94d8e3f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7188387894
|
ramonjd
force-pushed
the
fix/custom-palette-default-name-with-number
branch
from
d7f37ef
to
edb21db
Compare
|
Size Change: +113 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
ramonjd
changed the title
…olor n`, e.g., `Color n2` we should class this color as custom and not "Default", and therefore unsaveable. Checking the slug index against position index in accordance with getNameForPosition Renames "temporary" to "default" to be clearer about what the color/gradient is
Only when a user clicks "Done" will the name be assigned.
Fixed style attr linting by creating a new classname
ramonjd
force-pushed
the
fix/custom-palette-default-name-with-number
branch
from
94d8e3f
to
eb9be93
Compare
…sn't jump around as much
There was a problem hiding this comment.
Thanks for the PR! A couple quick thoughts from testing:
- This branch no longer shows the default (temporary) name in the input; is that a deliberate way to try to get folks to add their own name? Would it be possible/desirable to preserve the current behaviour?
- The gradient selector's initial state is
undefinedand it's possible to save that state:
and then apply it as a gradient to a block, which adds a style containing an undefined CSS custom property:
(This is strictly speaking unrelated to this changeset; the undefined gradient can also be saved on trunk if we give it a non-default name.)
| @@ -3,7 +3,7 @@ | |||
| ## Unreleased | |||
|
|
|||
| ### Bug Fix | |||
|
|
|||
| - `PaletteEdit`: ensure number changes to custom names are classes as non-default ([#56932](https://github.com/WordPress/gutenberg/pull/56932)). | |||
There was a problem hiding this comment.
Should be "classed"?
|
Thanks for testing this one.
It was a deliberate move to avoid the buggy, overly-complex way we assign default color names. The way the component decides if a color is a default one, and therefore able to be filtered out, is if the color and name are default values, e.g., The problem is, we can't discern between It's a bit of an edge case admittedly. The motive behind the PR was to only assign a default name if the user hasn't entered a name of their own AND they have changed the color. In order to make it more like trunk, maybe I could fiddle with the placeholder value so that it displays I'm thinking out loud here, but I'll take a stab at it.
That's not great, I'll fix. Thanks! |
|
Converting to draft while I review this and try to come up with an alternative. Ideally, it would be good to:
|
|
Thanks @ciampo @t-hamano Thanks to @tellthemachines, I realised this PR isn't quite ready. Interested to hear if you have ideas. 🍺 |
|
I'd like to spend some more time understanding this PR, but I think we should also consider scenarios where non-Latin characters are entered in the color name, whether or not you want to display the default name. This issue was reported in #51230, but trunk does not increment the number when consecutively adding palettes. 58baaeed67ea4af7d13313e71a8d2790.mp4On the other hand, this PR appears to generate the correct sequential number when I click "Done". So I think this PR is correct regarding the logic it is attempting 🎉 e09f7282725e5e511c9d90c68451ef88.mp4 |
Oh yes, definitely!
That's why I went with assigning a fallback name before saving the list and after discarding the defaults: it seemed more reliable. My idea to go further is show the generated name — "Color n" — in the placeholder only and not assign the name to the element item - that way we can check for |
|
Closing in favour of #54332 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
When a user edits a custom color/gradient name by adding digits to
Color n, e.g.,Color n2we class this color as custom and not "Default", and therefore unsaveable.Context: #56896 (comment)
Why?
The user has edited the name. It's no a longer default.
How?
Refactoring the UX:
TODO
Testing Instructions
i. a custom color/gradient without names, some with
ii. some using the default color/gradient with no name, and some with the default color/gradient with a name
Testing Instructions for Keyboard
Screenshots or screencast
This PR in action
2023-12-20.14.31.20.mp4