Skip to content
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

Conversation

aaronrobertshaw
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw commented Oct 13, 2020

Description

This is another pass at adding font style block support taking on board feedback from #25641

Changes Included

  • Adds block support for font styles
  • Allows for preset font style options and uses CSS variables
  • Updates navigation block to opt-in to font style support for easier testing

How has this been tested?

Manually tested using heading and navigation block.

Test Instructions

  1. Add a navigation block to a post and select it.
  2. You should see a new font style option under the Inspector Controls > Typography section.
  3. Select various font styles and confirm the block updates visually as you'd expect.
  4. With the italic style selected, inspect the selected block in dev tools and confirm that the block includes an inline style using var() and an appropriate CSS variable.
  5. Save the post and view on the frontend.
  6. The same font style choices should be reflected on the frontend block's inline styles.

Screenshots

FontStyleBlockSupport

Types of changes

Enhancement

Next Steps

  • Update tests in class-block-supported-styles-test.php if needed after approach confirmed.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@aaronrobertshaw
Copy link
Contributor Author

@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

@apeatling
Copy link
Contributor

apeatling commented Oct 15, 2020

This works well. I am noticing more space between the setting heading and control though that other settings:

Screen Shot 2020-10-15 at 1 46 51 PM

@aaronrobertshaw
Copy link
Contributor Author

This works well. I am noticing more space between the setting heading and control though that other setting

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.

Current Controls:
Screen Shot 2020-10-16 at 1 55 44 pm

@ItsJonQ
Copy link

ItsJonQ commented Oct 19, 2020

@aaronrobertshaw

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. Italic) to unset it.

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.

💡 Suggestion

A suggestion I have would be to add a Normal option to the group.

It also appears we're waiting for icons. I wouldn't really know what a Normal icon may look like either, haha.

Keeping with the "one must be selected" UI...

An alternative control we can use my be a SelectControl. This may be a simpler option :).

Then, the UI would look like this: (excuse the text-based art below)

Font Style
[ Normal           v ]

cc'ing @jasmussen for design thoughts!

@aaronrobertshaw
Copy link
Contributor Author

@ItsJonQ

Thanks for suggestion and explanation around segmented controls 👍

I've updated this to use a SelectControl now. It currently adds a "default" option (no style at all) so the font style can be more easily inherited. I'm happy for this to be omitted if there is a better approach. Likewise, this commit can be reverted if others prefer to continue with a ButtonGroup

@jasmussen
Copy link
Contributor

jasmussen commented Oct 20, 2020

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:

Screenshot 2020-10-20 at 08 31 27

As you can see, this is the font weight panel, which simply includes weight+italic at the bottom. I'm aware that italic and oblique are technically separate fonts with different designs, but I haven't been able to find a single Google font that includes an oblique version, so it seems wrong to surface it prominently.

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!

@aaronrobertshaw
Copy link
Contributor Author

Appreciate the feedback @jasmussen

I've updated this to a dropdown although it still currently has oblique as an option. I'm happy to remove it from the preset options though as it never quite felt right.

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.

This makes sense to me 🙂

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.

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.

@jasmussen
Copy link
Contributor

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.

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.

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!

@aaronrobertshaw
Copy link
Contributor Author

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.

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 theme.json. I see this as a pretty contrived scenario however and not worth missing out on the more streamlined experience you suggested.

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!

I'm confident we can merge the font weight and style options. I have a few ideas on that side of things.

@jasmussen
Copy link
Contributor

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 theme.json. I see this as a pretty contrived scenario however and not worth missing out on the more streamlined experience you suggested.

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.

Regular
Bold
— 
Regular Italic
Bold Italic
— 
Regular Oblique
Bold Oblique

?

@aaronrobertshaw
Copy link
Contributor Author

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?

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.

@aaronrobertshaw
Copy link
Contributor Author

During some internal discussions, a few possible issues with merging the font weight and font style options into one came up.

  • Is the user to be expected to know what font weight the text already is if they just want to make it italic?
    (They wouldn't see the current weight in that merged dropdown if it was simply the font weight set via theme CSS)
  • What happens if we only wanted to allow the application of font styles not font weights?
  • The implementation would be simpler and more flexible if separate.

I'd be inclined to leave font style separate to font weight at the moment although I'm happy to hear what others think.

@aaronrobertshaw
Copy link
Contributor Author

Latest updates include:

  • Removing the opt-in for font style support from the heading block
  • Moving the application of the font style inline style to the typography tools where it should have been

I think the main item still unresolved is deciding whether we actually merge the font styles with font weights or not.

@jasmussen
Copy link
Contributor

jasmussen commented Oct 22, 2020

  • Is the user to be expected to know what font weight the text already is if they just want to make it italic? (They wouldn't see the current weight in that merged dropdown if it was simply the font weight set via theme CSS)

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:

Screenshot 2020-10-22 at 09 39 12

... but perhaps combined with some progressive disclosure so you have to dig for the option, it might avoid getting undue prominence:

Screenshot 2020-10-22 at 09 42 23

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:

<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><path d="M14.5 7V5h-5v2H11v10H9.5v2h5v-2H13V7z"/></svg>

CC: @mtias

@aaronrobertshaw
Copy link
Contributor Author

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.

That is a better way of articulating it. I think keeping the ability to inherit the font weight is a good thing.

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 work avoid getting undue prominence:

I can agree with that. Would this be something that we can refine and iterate on?

I want to really emphasize that the above are mockups

Understood. It helps to see some of the potential directions we might take this.

@jasmussen
Copy link
Contributor

I can agree with that. Would this be something that we can refine and iterate on?

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.

@jasmussen
Copy link
Contributor

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.

@aaronrobertshaw
Copy link
Contributor Author

aaronrobertshaw commented Oct 23, 2020

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?

@jasmussen
Copy link
Contributor

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?

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.

Other than undo, will they have an easy option to remove a combo selection, e.g. "default" or "inherit", perhaps?

Yes, there should be a "Default" item in the top of the dropdown menu to let you clear any weight selection. Great point.

@mtias
Copy link
Member

mtias commented Oct 23, 2020

Yes, there should be a "Default" item in the top of the dropdown menu to let you clear any weight selection.

We have the ellipsis for clearing a specific typography field, no?

@aaronrobertshaw
Copy link
Contributor Author

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.

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.

We might want a different label than "weight"

Agreed. Something like "font appearance" would cover both weight and style although might be too generic. I'll leave that one with you 🙂

@jasmussen
Copy link
Contributor

We have the ellipsis for clearing a specific typography field, no?

Yes, that would be in addition to an unset value in the dropdown, though?

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.

Either one that accomplishes the unified weight-with-italics picker, I'd defer to you.

Agreed. Something like "font appearance" would cover both weight and style although might be too generic. I'll leave that one with you 🙂

"Appearance" might work?

@aaronrobertshaw
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants