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

Hooks: Add font style hook and use for navigation block #25641

Closed

Conversation

aaronrobertshaw
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw commented Sep 25, 2020

#25234

Description

Add block support flag and hook to set various font styles e.g. bold, italic, underline, strikethrough.

The font style controls are included within the existing typography controls along with those from the font size and line height hooks. They are simply a collection of buttons to toggle each style on and off.

How has this been tested?

Tests: packages/block-editor/src/hooks/test/font-style.js

Manually tested using navigation block.

Test Instructions

  1. Add a navigation block to a post and select it.
  2. You should see new font style buttons under the Inspector Controls > Typography section.
  3. Toggle the various style buttons and confirm the block updates visually as you'd expect.
  4. With a font style toggled on, inspect the navigation block in dev tools and confirm that the block includes a has-font-style in its css classes along with the style's specific css class as per below:
    • bold => has-bold-font-weight
    • italic => has-italic-font-style
    • underline => has-underline-text-decoration
    • strikethrough => has-strikethrough-text-decoration
  5. Save the post and view on the frontend.
  6. The same font style choices and class names should be reflected on the frontend.

Screenshots

FontStyleSelections

Types of changes

Enhancement

Next Steps

  • Write tests class-block-supported-styles-test.php

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 aaronrobertshaw changed the title [WIP] Hooks: Add font style hook and use for navigation block Hooks: Add font style hook and use for navigation block Sep 29, 2020
@aaronrobertshaw aaronrobertshaw marked this pull request as ready for review September 29, 2020 06:58
@glendaviesnz
Copy link
Contributor

This tested well for me in editor and front end. The only question I had was how we handle the font style option on the child if the style is set by the parent, eg. currently if the parent sets the font style to italic it is not possible to toggle it off in the child:

font-style

@apeatling
Copy link
Contributor

apeatling commented Sep 30, 2020

This works well, but as @glendaviesnz mentioned above, I think it should allow for overriding in the child. Also, I don't think a select dropdown is the right UI for this. I imagine there will be more font style options, and what if I wanted bold and italic? It should be a form of multi-select, or maybe even the same buttons as the block toolbar for consistency. I'm sure @shaunandrews has thoughts on this.

Edit: Looking at the CSS spec normal/italic/oblique are the only options for font-style, but in terms of the UI I'd expect we'd have more than this available?

@youknowriad youknowriad requested review from ItsJonQ and mtias October 1, 2020 08:00
@youknowriad youknowriad added [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Type] New API New API to be used by plugin developers or package users. labels Oct 1, 2020
@mtias
Copy link
Member

mtias commented Oct 1, 2020

We should model this around the typography tools as being defined in theme.json from the start to avoid another case of migrations. cc @nosolosw and @jorgefilipecosta for guidance on how to include

@aaronrobertshaw
Copy link
Contributor Author

@apeatling and @glendaviesnz It looks like there is a bit of a clash with this PR's approach using styles and the RichText component for the navigation link itself using <em> tags etc for formatting. It's a problem when wishing to opt-out of the italics style if chosen on the main navigation block as you point out.

The desired result is achievable by turning off italics on the main block and then manually applying that font style to each link except the one you wish to have regular style. Obviously, this isn't ideal, so if you have any ideas that would be great.

In terms of the UI and what options we make available, oblique, strikethrough, font-weight etc. that should be more straightforward. I'll explore switching the dropdown to something more akin to the block controls toolbar options.

@mtias
Copy link
Member

mtias commented Oct 2, 2020

Inline formatting and block level style are inherently different, even if the visual result is analogous — inline formatting should be seen as adding emphasis to text semantically (<em>) while changing the font style for the whole block is an aesthetic choice that carries no semantic meaning. We don't yet have font-style support in the typography tools, but it's marked as a task to get to.

@aaronrobertshaw
Copy link
Contributor Author

Thanks for the clarification @mtias

I'm a little confused, what are you referring to by "typography tools". I'm still trying to get a handle on the correct terminology and all the different features.

@mtias
Copy link
Member

mtias commented Oct 2, 2020

Sorry, the typography tools are the object controlling typography setting on a block level. It currently has font-size and line-height.

@aaronrobertshaw
Copy link
Contributor Author

Ok, that was my thinking and this PR was actually my attempt to add font style to the typography tools alongside font size and line height.

Without wasting too much of your time, if you get a chance to point me to where I've taken the wrong path, I'll try and get this fixed up.

Added `has-font-style` generic CSS class when there is a style selected.
Made attribtue injection more robust in case of undefined attributes.
Tweaked addAttribute function name to be more accurate.
@oandregal
Copy link
Member

👋 Took a quick pass at the code and one thing I've noticed is that we're conflating three different things (font-style, font-weight, and text-decoration) into one. From the point of view of the consumer of this API, it seems it's best to have them separated.

=> theme.json

"typography": {
    "fontStyle": "<value>",
    "fontWeight": "<value>",
    "textDecoration": "<value>"
}

=> block.json

"__experimentalFontStyle": true,
"__experimentalFontWeight": true,
"__experimentalTextDecoration": true,

Given these are 3 different things, it'd be good to do it in smaller steps / individual PRs so they can be discussed separately.


In terms of UI/Design tools, I'd welcome a design review (@noahshrader @ItsJonQ). There's also a related PR for font-weight that @jorgefilipecosta is working on and that takes a different direction (uses a select to show all the options instead of a boolean control).

@aaronrobertshaw
Copy link
Contributor Author

Thanks @nosolosw for taking a look, I appreciate the feedback and guidance.

Separating the font style, weight and text decorations might be the best option. My questions are only around the UI and how we avoid overcomplicating that. A design review to get some direction on the UI sounds like a good idea 🙂

I went down this path after some earlier discussions around allowing multiple options under the banner of "font styles" within the UI and trying to keep some consistency with the block toolbar. It was the latter that lead to the boolean toggle for a bold font weight.

I'm happy to split this up and change approach however we need. Ultimately, we're hoping to be able to leverage these features in building block patterns.

cc @apeatling

@aaronrobertshaw
Copy link
Contributor Author

There is another issue with the current approach. It is based upon applying CSS classes to a block. The styles from these classes are inherited by the block's children and can prevent the user from having the expected control over text within a RichText component.

For example, if the italic font style is applied, then within the RichText component there is no way to override or opt out of the italics style as that component will semantically apply inline formatting via <em> tags.

Any suggestions on how to tackle this would be appreciated 🙂

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi tested this PR and it worked as expected and the code also looks good 👍

👋 Took a quick pass at the code and one thing I've noticed is that we're conflating three different things (font-style, font-weight, and text-decoration) into one. From the point of view of the consumer of this API, it seems it's best to have them separated.

I agree with this point.

In this PR we have a style shape "style":{"typography":{"fontStyles":{"italic":true,"bold":true,"strikethrough":true}}}}.
Normally a style attribute directly maps to CSS output e.g: I would expect something "style":{"typography":{"fontStyle":"italic"}}} to map to CSS font-style: italic. I think breaking that assumption may make somethings on the global styles infrastructure not work as expected.
For example, to allow themes to set the styles on theme.json normally we add a mapping

'fontSize'                 => array( 'typography', 'fontSize' ),

The mapping says values under typography.fontSize are output as CSS to the font-size property. With the structure from this PR, a mapping like that would not work.

I guess it would be preferable if we have distinct attributes for font-style, font-weight, and text-decoration, and a UI to allow changing the different attributes. I guess as part of the UI we may add an interface similar to this one where there is some kind of typography presets and enabling them may change multiple typography attributes. But the attributes would be stored independently of each other.

For example, if the italic font style is applied, then within the RichText component there is no way to override or opt out of the italics style as that component will semantically apply inline formatting via tags.

Any suggestions on how to tackle this would be appreciated 🙂

It seems like the current UI is very similar to the formats UI and in fact, users may become confused because they may think a format is being applied and then they can not remove it at the local level. But I guess if the UI not similar to the formats UI e.g: a different control for font style, weight, and decoration I think the expectation the format affects the typography setting is removed.
In the end, it is the same that happens currently, if a theme makes some text area or paragraph italic the there is no way the user can remove the italic using the formats. Personally I think this is not a big issue.

* was chosen so that the CSS classes were more explicit in what they would be
* setting.
*/
export const fontStyleClasses = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this PR we are using classes for the presets like we currently do in colors. It seems there are discussions related to if we should continue to use CSS classes or move to use of CSS variables on PR's #24978 and #24868 for font weight and font family I end up using the CSS variable approach.

@aaronrobertshaw
Copy link
Contributor Author

I'll close this PR in favour of three new PRs, one each for adding font style, text decoration and text transform block supports.

Drafts are currently under way; #26050, #26059, and #26060

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Type] New API New API to be used by plugin developers or package users.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants