-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PaletteEdit: refactor custom color/gradient name assignment #56932
Conversation
Flaky tests detected in 94d8e3f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7188387894
|
d7f37ef
to
edb21db
Compare
Size Change: +113 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
…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
94d8e3f
to
eb9be93
Compare
…sn't jump around as much
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
undefined
and it's possible to save that state:
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be "classed"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Good catch.
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 |
What?
When a user edits a custom color/gradient name by adding digits to
Color n
, e.g.,Color n2
we 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