Remove the duplicated title in background, text and link views in Global Styles #35583
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
youknowriad
added
[Type] Enhancement
|
Size Change: +116 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
In a straight before and after comparison, I see this before: And this after:
You'll notice I didn't even discover the swatch in the header, because the white swatch didn't have a border so it blended in. So just in removing the extra chip, definitely. I want to question whether we need to move it to the header at all — what value does it bring? There's a check indicating the color you've chosen. I'd honestly think it fine to remove it from regular panels as well:
|
Yeah I didn't add it at first but it's important for custom colors I think |
Ah, good point. @pablohoneyhoney I feel like you may have thought about this. |
|
We have a design for this:
The color swatch is also what opens the color picker. |
youknowriad
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
I updated the pickers to match the designs. I think the indicator as implemented right now works well for some colors but not for others, I wonder about if we should add a border or something like that. @jasmussen give it a try and feel free to tweak it. |
|
Took it for a spin, and I'm very possibly doing something wrong with my theme, but my stylesheet appears to not be loading into the editor, despite using I do agree with the borders around color swatches. I like the borderless look, but it feels like someting to tackle separately so that at least the white swatches have a border. The custom color picker looks mostly the same as in trunk, or am I missing nuance? |
Yes, I didn't touch this one just removed the "custom" link and replaced with the color box. |
|
Nice, I didn't find myself missing the other piece 👍 👍 |
ciampo
reviewed
Oct 18, 2021
| <> | ||
| { label } | ||
| { !! value && ( | ||
| <ColorIndicator colorValue={ value } aria-label={ ariaLabel } /> |
There was a problem hiding this comment.
Would deleting the code that computes an accessible color label be an accessibility regression?
There was a problem hiding this comment.
I don't know for sure but I don't feel like it since that label is only read when the element is reached which is not the case here but.
There was a problem hiding this comment.
Ok. I guess we still need to work on replacing / consolidating the color indicator components (tracked in #35093), so probably adding solid accessible text should be done there as well
|
@ciampo Something a bit unrelated but I noticed that some CSS in JS styles are not working properly in the site editor (try applying an inline text color to some characters, you'll see that the color indicator in the popover has some CSS issues, I found related to HStack) |
I'll try to have a look later today, but there's a chance I won't manage (since I'll be AFK from tomorrow until November 8) [CC'ing @griffbrad @mirka and @diegohaz] |
|
What's the remaining things here? |
|
Thanks for your work here Riad! I've not reviewed yet the code, but started testing and noticed the below when no color is set.. Screen.Recording.2021-10-21.at.3.15.43.PM.mov |
|
@ntsekouras yes, I did that on purpose, what would you suggest? |
I guess I'd expect to see some kind of icon in GS list (0.15 in my above video) with `background, color, etc to indicate a color is not set 🤔 For the bar with the color preview, I think it's a bit big but I also think that it's better this way to avoid the layout shifts on set/unset. Also the |
|
@ntsekouras got it 👍 I used the same pattern I've used on the custom picker to fix that. |
ntsekouras
approved these changes
Oct 25, 2021
7 tasks
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.
Related #34574
This does two small things: