-
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
Block Support: Add font styles support using CSS variables #26050
Block Support: Add font styles support using CSS variables #26050
Conversation
9b8df64
to
2809cb4
Compare
@noahshrader or @ItsJonQ It would be great if we could get a design review for the Font Style selection controls UI when you get the chance. My apologies if there is someone else I should have pinged for this, you drew the short straw as you were mentioned on #25641 |
I've tidied up the controls. As per the suggestion on the text decoration/transform PRs I've added the use of an icon if one is available. Making all the button use icons will avoid having to handle i18n of the preset font style names. We will need an icon for the oblique style. Assuming we want to offer support for that. Only having a single button for the italic style looks odd though. |
Haii!! I checked out the update. I was initially a bit confused with how to reset/remove the setting. I then realized (thanks to your GIF) that you have to click the option again (e.g. With Segmented Controls (or UI like it, e.g. a button group), there's usually one item always selected. You just toggle between the items. 💡 SuggestionA suggestion I have would be to add a It also appears we're waiting for icons. I wouldn't really know what a Keeping with the "one must be selected" UI... An alternative control we can use my be a Then, the UI would look like this: (excuse the text-based art below)
cc'ing @jasmussen for design thoughts! |
Thanks for suggestion and explanation around segmented controls 👍 I've updated this to use a |
Thanks for the ping, and thanks for all the work here! My first instinct was to ponder: do we need to show font style as a separate control? And the second instinct was similar to Q's — this should be a dropdown. Here's what Figma does: As you can see, this is the font weight panel, which simply includes weight+italic at the bottom. I'm aware that By including the font style in a "font weight" dropdown, it's one place for people to look for the aesthetic font change, and it groups conceptually related items together. This seems backed up by Matías comment in #25641 (comment), which suggest that this is a decorative choice that goes outside the inline semantic meaning. If we need to ship this PR before being able to ship the font-weight feature, you could call the dropdown "Font Style", but when we then are able to ship the font weight, we should combine the dropdowns into one. I would think it a mistake to ship dropdowns for both weight and style. Nice work! |
Appreciate the feedback @jasmussen I've updated this to a dropdown although it still currently has
This makes sense to me 🙂
The need driving this change is to be able to leverage it while building block patterns. That said, I do not think there is a great rush for this over the next couple of weeks. Our current focus is refactoring the Gallery block and making improvements around premium content blocks. I am not sure how close the font weight block support is to shipping. Perhaps @jorgefilipecosta would like to weigh in on this? If we decide to ship font style block support first, I'm happy to create an issue for merging the options and pick it up once that can actually happen. |
Fast work! On the question of whether to include both italic and oblique as options, I'm happy to defer to someone with a strong opinion. But based on cursory research (this suggests italic is best, this suggests actual obliques are rare, and this goes into more depth) I'm not sure it's worth it to include both, and that notably the difference is likely to be hard to perceive for users.
Just wanted to make sure I noted that I'm only commenting on the user facing aspect. I trust the technical underpinnings (global styles and json structure) to you! |
Well, you can say I cheated 😄 I pushed a commit switching to a dropdown not too long before your feedback. That's the only reason oblique was still included. I agree that it provides very limited value and could potentially confuse users. I'll remove it from the preset style options. For interests sake, if we do not merge font style together with the font weight options, it would be possible for a theme developer offer an oblique custom font and the oblique font style. After adding the custom font, they could simply add the oblique font style to the presets within their
I'm confident we can merge the font weight and style options. I have a few ideas on that side of things. |
I don't think the following would necessarily be a good experience, but if a theme truly wanted to offer both oblique and italic combinations, wouldn't the combined weight dropdown just be much longer? I.e.
? |
That's true, they could. Given the number of font weights on offer via that PR it would be a huge dropdown as you mention. I didn't consider that as much of an option. |
During some internal discussions, a few possible issues with merging the font weight and font style options into one came up.
I'd be inclined to leave font style separate to font weight at the moment although I'm happy to hear what others think. |
Latest updates include:
I think the main item still unresolved is deciding whether we actually merge the font styles with font weights or not. |
This is a good observation, and deserves decompression. Because it seems so obvious to go with a unified dropdown when virtually all other design apps do that. So the reason it may make sense to keep the font style separate for us, is that you might want to create a pattern that inherits the font and font weight, but italicizes it. If you had to choose the weight at the same time, you wouldn't be able to inherit that. I don't love that it results in the following: ... but perhaps combined with some progressive disclosure so you have to dig for the option, it might avoid getting undue prominence: I want to really emphasize that the above are mockups, they are tentative explorations into how the controls could be laid out. Please take it as inspiration for experimentation, for example again in the Navigation block. Here's the new non-italic if you need to try it out:
CC: @mtias |
That is a better way of articulating it. I think keeping the ability to inherit the font weight is a good thing.
I can agree with that. Would this be something that we can refine and iterate on?
Understood. It helps to see some of the potential directions we might take this. |
Potentially. My brain is still going on this one :) I'd like us to feel with some confidence that the approach is correct, that we have the right puzzle pieces. Once we have that, it's probably fine to build this out in a limited capacity on the navigation block and learn from it, provided we are comfortable revisiting the visuals if/when they change and improve. I wish there was a simpler interface for the font style, one that didn't take up a whole separate control and looked so much like the Italic button on the regulard block toolbar. But I'm not quite seeing it. |
Coming back to this one, and despite what I wrote yesterday about inheritance, after discussing it with @mtias, I'm back to my initial assessment that font-style should not be a separate control, but should be integrated into the singular font-weight dropdown, like others do. We might want a different label than "weight", but it's still the best user experience. The primary reason is that there's almost never a reason to just italicize a whole block. The use cases are primarily for appliance blocks like Navigation or Buttons, and in those cases, it seems fine to choose a specific weight at the same time. |
In the situation where no previous selection of a font weight and style combo has been made and the font weight has been inherited via CSS. When the user wants to italicize it, how do they know which weight and style combo to select to keep the weight the same visually? Are we happy accepting they might need to take a few goes selecting the right combo? Other than undo, will they have an easy option to remove a combo selection, e.g. "default" or "inherit", perhaps? |
Because this use case seems so rare, it seems fine that they'd have to pick between the options available to choose the one that looks good. The alternate route, with a separate component, is just that much more UI to have to handle, which especially in the global styles interface will add up.
Yes, there should be a "Default" item in the top of the dropdown menu to let you clear any weight selection. Great point. |
We have the ellipsis for clearing a specific typography field, no? |
Thanks for the explanation. Now we have a direction, do you think it best to continue with this PR or #24978? I don't mind which.
Agreed. Something like "font appearance" would cover both weight and style although might be too generic. I'll leave that one with you 🙂 |
Yes, that would be in addition to an unset value in the dropdown, though?
Either one that accomplishes the unified weight-with-italics picker, I'd defer to you.
"Appearance" might work? |
Closing this in favour of #26444 that combines adding support for both font weight and font style, while combining the UI controls as discussed here. |
Description
This is another pass at adding font style block support taking on board feedback from #25641
Changes Included
How has this been tested?
Manually tested using heading and navigation block.
Test Instructions
var()
and an appropriate CSS variable.Screenshots
Types of changes
Enhancement
Next Steps
class-block-supported-styles-test.php
if needed after approach confirmed.Checklist: