-
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
Add labels to the global styles variations #39322
Conversation
Size Change: +464 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
You're so fast it's hard to keep up! The initial mockup of labels actually made me think that, their utility aside, the primary challenge with these variations remains that they all look alike. And in that light, they risk actually adding a little noise, and distract from the main problem of diversity in the appearance. I think it could be worth revisiting how the card is composed; in a little longer term I'd love to see everything from font pairings, various border radiuses, maybe even photo elements as part of it. And indeed the title of the variation could be a part of the card itself, perhaps on hover, letting the resting state of the card remain a "style stamp". I don't completely love the following fake animation, but just to high level illustrate the idea, here's what a style called "Stratus" could look like when hovering: |
Thanks for trying this out @youknowriad! ⚡ I like @jasmussen's idea of using animation to incorporate the label/swatches — seems that could help resolve both of the main points discussed in #38918. |
@youknowriad Can you add more specific instructions? I need to know what to click and search for to find this section. I'm not sure I can make it there since FSE is still in super rough shape for screen readers but I'd like to give it a try. Thanks. |
To make this translatable, the i18n schema needs to be updated here and in WP-CLI |
@alexstine I've detailed the testing instructions in the PR description a bit more, let me know if there's anything missing, I'm happy to detail more. |
I've been thinking a bit about @jasmussen's proposal above. I like it a lot but I also fear that it doesn't solve the problem fully. Basically, the issue is that unless you hover the preview, you might not notice a difference between the variations. I'll give it a try but I wonder if there's a middle ground where we have both: the hover effect and the visible small label |
This feels like enough of a limitation of the card design itself that it feels worth addressing. Foreground and background colors feel solid enough, but perhaps the two dots show something other than text and link. The label is valuable and should definitely be visible at least on hover. But on its own it's not going to make things "scannable" or inviting, which for style variations is arguably the most important problem to solve. |
Unfortunately, this is one of those cases in which we need to update the code in addition to the i18n schema. @swissspidy I've prepared an initial PR for the wp-cli at wp-cli/i18n-command#306 I'll follow up with the required changes for Gutenberg. |
@oandregal before committing these, I want to check with folks whether we want this to be called "name" or "title" or something else :). |
@oandregal Note that you need to update https://github.com/WordPress/gutenberg/blob/f9c2838a0b8be94fcf4e51b0a831d4629eaf24d7/lib/compat/wordpress-5.9/theme-i18n.json as well with the same changes.
I see how using |
I still need to prepare the tests for the wp-cli, so no worries, we can wait until this PR lands. I'd also like confirmation about what we want the context to be (what's shown to translators). I went with "Name of the style variation" but thought I'd ask if we envision this field to be used for something else, in which case it could be worth using a more generic approach such as "Name of the style source", or something along those lines.
👍 |
@youknowriad I found the Styles button and it was expanded but that button does not manage focus. Once I arrive in the sidebar, what is the button I am looking for? |
@alexstine Inside the sidebar, the button should be labeled "Other styles" (that's actually a visible label inside it) |
@youknowriad Still don't see it. When I make it to the "StylesBeta" sidebar, these are my options.
Any thoughts? |
@alexstine Do you have a "styles" folder in your theme with some variations. (I'm mentioning this on the testing instructions)
|
packages/edit-site/src/components/global-styles/screen-style-variations.js
Outdated
Show resolved
Hide resolved
d2f7038
to
0d1fbe3
Compare
I feel like there are still some missing pieces, perhaps to the style variations which I got from here. But even with style variations that all have a white background (is that intentional @kjellr or do I have old files?) this already increases the diversity.
There are some metrics here and there that I can likely dive in and fix, but I think that can work fine separately to this PR so nothing that needs to block it. Which means this one feels close to landing! |
Sure, for all "Aa"s or just the heading one (main one)
Ideally each variation defines a "name" property in the json, the filename is just a fallback
I'll try |
For the main card one, and for the title shown on the hover.
Ah, perfect! |
@jasmussen I think I addressed all the points. |
This is already looking way better to me: Focus works great here 👌 — so well in fact, I feel it validates the hover idea. The duplicated greens and pinks in the attached is because the variation duplicates the same color, so probably something to fix there. I'd like to dive in and tweak some of the metrics of the second frame, but I need a break from the keyboard. Would you prefer I do this in a followup PR separately? I'd be happy to, and I'd also be happy to try this one as is, pending any thoughts from @kjellr and @critterverse. |
I think a follow-up would be great mainly to unblock some related work in i18n. cc @oandregal I'm still looking for confirmation that "name" is the best property name for the label, I don't know, "title" or "label" could be valid alternatives. cc @mtias @mcsf |
Here's the current state of the Twenty Twenty-Two variations (WordPress/wordpress-develop#2440), tested against this PR: In general (and aside from the fonts not working yet... if someone has a sec to help look into that, I'd appreciate it!), it feels pretty good! The mint + pink color variants are showing the same circle swatch color twice, but they do define it twice at the moment — I'm probably going to go in and adjust that on the theme side anyway. |
Awesome, this is working so much better! Thanks @youknowriad ❤️
This sounds great, @jasmussen. I'd also like to follow up with a simplified animation suggestion for transitioning between the two frames that could be addressed separately as well. |
@kjellr I can also filter duplicates in colors. |
@kjellr Actually, I just tried filtering duplicates and I ended up with just a unique color dot for both these variants which felt a bit weird as a result. So maybe duplicates are fine for now. |
b15fbc8
to
78089c8
Compare
That's fine, we can always re-address later. |
Hey all, I shared a revised version of the color fill animation approach in #39700. It includes the sliding color bars but reduces the complexity of the hover frame to only show the variation title as Heading text, which I think should help with the animation feeling too weighty. |
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.
Looks good, thanks Riad! Let's land and iterate.
Status on translating the |
@beckej13820 Not exactly the same idea but I shared a design in #29388 (comment) recently that would introduce more flexibility for style variations by offering granular Global Styles saving options. |
What?
closes #38918
This PR adds "labels" to the global styles variations shown in the site editor sidebar.
Why?
Something the colors and the previews are not enough to differentiate between the available variations. This PRs allows theme authors to provide a name for each style variation to disambiguate between them.
How?
Theme authors can add a top level
name
property to thejson
file for each variation. If no name is provided, we fallback to the file name (without the extension).I'll be needing some help here to make this new property translatable. cc @oandregal @swissspidy
Testing Instructions
theme.json
like file (same format as theme.json but different name potentially) to thestyles
folder of your themename
property in the JSONstyles
folder of your theme. Each style variation should have the proper label corresponding to the "name" property in the theme.json file.Screenshots or screencast
Screen.Recording.2022-03-09.at.5.51.31.PM.mov