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

Paragraph: Add LineHeightControl + Attribute #20775

Merged
merged 30 commits into from
Mar 27, 2020

Conversation

ItsJonQ
Copy link

@ItsJonQ ItsJonQ commented Mar 10, 2020

Description

Screen Shot 2020-03-10 at 13 34 59

This update adds a new lineHeight attribute to the core Paragraph block. Accompanying it would be a new dedicated (and experimental) LineHeightControl, which is provided by @wordpress/block-editor.

How has this been tested?

This has been tested on local Gutenberg on multiple themes, from 2011 -> 2020.

Screenshots

Screen Capture on 2020-03-10 at 13-32-25

GIF demonstrates the live control edit -> rendering for line heights.

Types of changes

A majority of the updates in this PR hopes to improve the relationship + setup of attributes and controls.
A goal for LineHeightControl is to be implemented with minimal code changes for a Block's Edit/Save components.

LineHeightControl

Taking a look at the Paragraph edit.js, the new LineHeightControl has been added under InspectorControl without requiring any props.

The component is able to retrieve the appropriate attributes and setAttributes on it's own. This workflow makes it much simpler to add controls to Edit components - especially if the control requires multiple moving parts.

useBlockEditProps()

This new experimental hook from @wordpress/block-editor . This magical hook (which leverages other new experimental hooks) is able to retrieve a block's attributes and setAttributes without requiring anything to be passed in.

The new LineHeightControl relies on this hook as the getter/setter mechanism for the lineHeight value.

If folks are 👍 with this approach, I can see it being extremely useful for attributes x controls moving forward 🙌

Unitless

To keep things simple, the line-height value will be unitless and works as a multiplier to the Paragraph's font-size. This more hands-off approach allows for greater flexibility for font-size x line-height.

Control Styles

Some @emotion packages were added as a dependency to @wordpress/block-editor to enable CSS-in-JS style rendering for LineHeightControl. These are the same packages used internally within @wordpress/components.

UX Interactions

To improve the UX for LineHeightControls, I implemented some logic to better handle incrementing/decrementing values from the start.

For example...

If you're adjusting line-height for the first time, and you press UP, the value would be 1.6, rather than the default input behaviour of 0.1.

Alternatively, if you press DOWN for the first time, the value would be 1.4, rather than 0.

This is because it uses a smart default of 1.5. That being said, 1.5 isn't actually applied. There is no line-height style set until the user makes a change.

Afterwards, incrementing/decrementing works as normal.
Removing the value would reset it entirely.

During development, I found that the above behaviour adjustment was key. Without it, the initial increment (UP press), would cause all of the text to collapse, as it would use a line-height value of 0.1.

Tests

I would love to write tests for this new control. I attempted to do this, but I wasn't able to successfully mock the new useBlockEditProps hook, which technically could be replaced with useState. Jest kept complaining 😅

Closing Thoughts

Lots of changes in this PR! I'm open to anything. On the surface, adding a new line-height property to Paragraph is a very minor thing. The bigger goal though would be to systematize and make it easier for blocks (both core and 3rd party) to set up attributes and render their respective controls.

Thank you! 🙏

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.

Partially takes care of:
#20339

@ItsJonQ ItsJonQ added [Block] Paragraph Affects the Paragraph Block [Package] Block editor /packages/block-editor labels Mar 10, 2020
@ItsJonQ ItsJonQ self-assigned this Mar 10, 2020
@ItsJonQ ItsJonQ requested a review from mtias March 10, 2020 18:02
"customFontSize": {
"type": "number"
},
"direction": {
"type": "string",
"enum": [ "ltr", "rtl" ]
"enum": [
"ltr",
Copy link
Author

Choose a reason for hiding this comment

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

Prettier autofixed these ones

@github-actions
Copy link

github-actions bot commented Mar 10, 2020

Size Change: +444 B (0%)

Total Size: 856 kB

Filename Size Change
build/annotations/index.js 3.45 kB +1 B
build/block-editor/index.js 102 kB +341 B (0%)
build/block-editor/style-rtl.css 11 kB +20 B (0%)
build/block-editor/style.css 11 kB +20 B (0%)
build/block-library/index.js 110 kB +17 B (0%)
build/block-library/style-rtl.css 7.49 kB +15 B (0%)
build/block-library/style.css 7.5 kB +16 B (0%)
build/blocks/index.js 57.5 kB -1 B
build/deprecated/index.js 772 B +1 B
build/edit-post/index.js 91.2 kB -2 B (0%)
build/edit-site/index.js 6.73 kB +1 B
build/edit-widgets/index.js 4.43 kB -1 B
build/editor/editor-styles-rtl.css 423 B -5 B (1%)
build/editor/editor-styles.css 426 B -5 B (1%)
build/editor/index.js 42.8 kB +12 B (0%)
build/keycodes/index.js 1.7 kB +13 B (0%)
build/plugins/index.js 2.54 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 998 B 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-library/editor-rtl.css 7.22 kB 0 B
build/block-library/editor.css 7.23 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/components/index.js 190 kB 0 B
build/components/style-rtl.css 15.8 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 6.21 kB 0 B
build/core-data/index.js 10.6 kB 0 B
build/data-controls/index.js 1.04 kB 0 B
build/data/index.js 8.25 kB 0 B
build/date/index.js 5.37 kB 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/style-rtl.css 8.43 kB 0 B
build/edit-post/style.css 8.43 kB 0 B
build/edit-site/style-rtl.css 2.91 kB 0 B
build/edit-site/style.css 2.9 kB 0 B
build/edit-widgets/style-rtl.css 2.57 kB 0 B
build/edit-widgets/style.css 2.57 kB 0 B
build/editor/style-rtl.css 3.38 kB 0 B
build/editor/style.css 3.38 kB 0 B
build/element/index.js 4.44 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 6.95 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.93 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.49 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.84 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.01 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 781 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.5 kB 0 B
build/server-side-render/index.js 2.55 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.01 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@ZebulanStanphill
Copy link
Member

I am very much against this going in as a control for individual blocks. In my opinion, this is pure bloat; an option that gives too much flexibility and tries to solve a problem the wrong way. Any line height option should be handled at the global styles level; and I don't mean changing this for every Paragraph block... I mean this should be a setting at the same level as a site wide typography scale.

There is no good reason to add this kind of control to individual blocks like this. If a user is needing to change the line height on a single block, then that's a sign that the global styles were not good enough to begin with. The problem should be solved at the source, not with a band-aid like this. Please, do not add this control to the Paragraph block (or any block, really).

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Mar 10, 2020

I should note that I'm fine with adding a component for this that could be used in global styles; I'm just extremely against this kind of control being provided at the individual block level.

@mtias
Copy link
Member

mtias commented Mar 10, 2020

I disagree — most of the patterns being designed cannot be accomplished without control over line-height at the block level. If there is control over font-size, whether exposed to users or not, there should be control over line-height as they go hand in hand.

image

This is entirely complimentary to global styles handling a typography scale, the theme defining its own defaults, and so on. The block itself needs to understand the attribute, so this is a step in that direction.

It should be made easy to disable globally like other customization tools, of course, and we could debate the argument of whether it should be exposed by default or implicit on any defined pattern.

@ZebulanStanphill
Copy link
Member

@mtias Why not just make line heights part of the font size presets? We should definitely be encouraging the use of the font size presets rather than custom pixel sizes, and if we already have presets, why not automatically set the appropriate line height?

(These font size + line height combos could be defined at the theme level or at the global styles level.)

We already add padding when selecting a background color on various blocks, so the precedent for such a thing is already there. The user shouldn't have to be responsible for selecting the right line height to go with a font size. They shouldn't even have to think that much about the font size they want to use. They should be able to pick from a few fine-tuned presets and have it work by default with no extra fiddling around.

@mtias
Copy link
Member

mtias commented Mar 10, 2020

I think we are mostly talking about the same thing, in a way :) For visual effect, there will still be cases where you need specific control over it.

image

This doesn't mean that we cannot absorb most common cases in pairs (this we should do regardless for the main font-sizes) and global styles / theme, as you say, but it does mean we need the concept of its control at the block level somehow existing too.

@ZebulanStanphill
Copy link
Member

@mtias I think I get the idea of that example, but even in that situation, such an intentional visual effect is likely to be used in multiple places, and so something like that example would be far more suited to a block style variation for the Paragraph block (and/or Heading block).

Intentional stylistic choices like that should be the responsibility of the website designer, and good design reuses parts to create a cohesive whole. Therefore, style variations seem like the proper way to implement this.

By adding a line-height control to individual Paragraph blocks, you encourage one-off modifications. You bring to the forefront a "solution" to a problem, but it's the wrong solution in nearly every situation. You give the content creator more power, but also more responsibility... responsibility that what supposed to be handled by the site designer (whether that be the theme author or the person managing the global styles or both).

Individual block options should be designed around carefully crafted presets set by the designer, thus keeping the responsibility of design with the designer, not the content editor.

I recently read a very interesting blog post that points out how Gutenberg seems to be blurring the lines of responsibility with its increasing amount of style options on individual blocks.

But it doesn't have to be that way. We already have the block style variations API, and global styles are going to become a thing. Only weird one-off cases are not handled, and one-off cases should not be encouraged, in my opinion.

I think a better solution to this problem would be to provide an easier way to create/register a style variation. We could provide a visual interface for creating style variations so people don't have to write CSS; in that UI, there can be a line height control, as well as a zillion other advanced style options. But in the end, that style variation will be just a class applied to a block. There's no need to add a line height attribute to any block.

@ZebulanStanphill
Copy link
Member

Additionally, it follows based on my reasoning that the custom font size option probably shouldn't even exist for individual blocks. That may be a controversial stance, and it's definitely useful right now since global styles are not yet implemented and people can't easily create style variations, but in the long term, the custom font size option is not something that should be exposed for individual blocks. In the long run, custom font sizes should be opt-in, not opt-out. As it is, themes have lost control over font size consistency.

At least in the case of font sizes, the custom option is somewhat buried, with the preset options presented first. The same can not be said of this line height control.

@chrisvanpatten
Copy link
Contributor

I know this is probably going to come with the global styles work, but it'd be neat to see a "make default" option, so if you change the setting you can persist it back to the global configuration.

I do strongly feel like this would be a good candidate for the features API proposed in #20623, or at the very least something with the theme supports we currently have (add_theme_support( 'disable-custom-line-height' ) to mirror add_theme_support( 'disable-custom-font-sizes' ) and such).

@ZebulanStanphill
Copy link
Member

I agree that at the very least, themes should be able to disable this feature similar to custom font sizes and custom colors.

Also, another thing I wanted to point out: in the aforementioned stylistic flair example posted by @mtias, the person wanting to do that could just as easily have wanted to also use a different font weight or letter spacing. Should we be adding font weight and letter spacing controls to the Paragraph block? I don't think so.

@mtias
Copy link
Member

mtias commented Mar 10, 2020

But in the end, that style variation will be just a class applied to a block. There's no need to add a line height attribute to any block.

@ZebulanStanphill this is not necessarily the case. The actual appearance attributes need to make it to the block as attributes, otherwise we cannot ensure true WYSIWYG. Also mobile apps don't understand class names with arbitrary CSS attached, they will need to consume agreed upon attributes. The way global styles would work is by traversing the actual appearance attributes within the editor so that they can be consumed at the block level as well. This is paving the way for that, if seen beyond the mere presence of the line-height control on the block, as it makes the block understand an attribute. The presence of a control by the block is just an artefact. We also need to see how all of these should eventually be grouped in a style panel on the block, including ways to make changes the default (or save style variations) but all these layers require we build the tools, otherwise we are chasing ghosts.

By adding a line-height control to individual Paragraph blocks, you encourage one-off modifications. You bring to the forefront a "solution" to a problem, but it's the wrong solution in nearly every situation. You give the content creator more power, but also more responsibility... responsibility that what supposed to be handled by the site designer (whether that be the theme author or the person managing the global styles or both).

There's a multitude of use cases (one of pages, brochures, eclectic presentations) and we should not force our own preference on what site design should be on all people. Most block libraries have added these controls, but the implementation ends up being generally done through inline styles which hurts the user in the longer term. By taking on that mantle we can ensure consistency and build the right levels of control, which is one of the main reasons to do this in core, to avoid fragmentation and ensure the user stands on solid ground.

Site builders will be able to retain full control by not enabling / disabling certain appearance attributes. Once it's all properly codified, that should be an easy configuration set globally and cascading properly to blocks because they understand what to do with the attributes. This is also true if we want global styles to transpire to third-party blocks.

It's important to separate the fact getting the controls in place doesn't mean that everything will be exposed to end users. Most of the design tools, including global styles, theme editor, etc, are likely going to be admin level to begin with. The reality remains that global styles cannot work without also building the tools, and starting on individual blocks is more ergonomic than trying to solve everything at once at the global level. It also happens that patterns need more fine grained control over these attributes.

but it'd be neat to see a "make default" option, so if you change the setting you can persist it back to the global configuration.

@chrisvanpatten agreed, this was in one of the original block style variations issues as one of the future tasks.

Also, let's move the main conversation to the issue: #20339 !

@chrisvanpatten
Copy link
Contributor

chrisvanpatten commented Mar 11, 2020

One thing I want to observe is in regards to useBlockEditProps(). This is just an observation, not intended to be firm or anything — I'm still formulating my own thoughts and thinking out loud here.

As noted in the original post above, this is indeed a magical hook! I've been wondering for a while if this type of pattern might crop up.

I think it's a pattern that clearly has its use-cases — I fully switched our codebase from withState to useState because of how much I like hooks — but I'm also wondering about the potential impact on clarity and readability. One thing I really like about the XyzControl pattern in Gutenberg is the simplicity, consistency, and flexibility of the onChange and value props. It's easy for a beginner to grok and makes the code a little more clear. You pass in block attributes and setAttributes from your Edit component… it's very easy and clear where the data comes from and where it goes.

As awesome as hooks are, they do mask away some explicitness in favor of magic. And as great as magic can be, I think it's important to balance alongside readability and clarity.

Again, this isn't to say that we should go one way or the other… just wanted to share some musings that I've had recently that this hook brought to mind in particular.

I think technically the work is super interesting here, and seems like a great place to trial some exciting new APIs!

@mtias
Copy link
Member

mtias commented Mar 11, 2020

As awesome as hooks are, they do mask away some explicitness in favor of magic. And as great as magic can be, I think it's important to balance alongside readability and clarity.

Yes, this is a tricky thing to balance. Explicitness is sound when it comes to the main attributes of a block, but when we start dealing with shared attributes, it becomes unnecessarily verbose and tedious to add support. Adding support for text and background color means adding four new attributes at the moment! That's why I asked to explore the notion of implicit attributes as well: #20308

@karmatosed
Copy link
Member

First, I am super excited to see this. There is a strong case for line-height to create designs and really adjust to make the perfect layout that people envision in their head.

@ZebulanStanphill whilst I understand your point of view on this, I would ask you to consider a few points. If we are to truly give control, I don't think a range hits the right spot. There are many cases where more granular control really is what is needed. For sure we should offer strong defaults, that is where ideas like having a range control for typography scale come in.

We do need to offer some tools we don't right now to empower people to create. This doesn't always work per block, sometimes we're going to need to do globally, sometimes per block. However, we do need to open up the tools we offer to provide a powerful kit to create what people really want. It's exciting if we do that, because there are going to be so many opportunities for incredible things to be made.

@karmatosed karmatosed self-requested a review March 11, 2020 17:17
Copy link
Member

@karmatosed karmatosed left a comment

Choose a reason for hiding this comment

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

Testing this and giving a design review, it's great. I am not giving a code review so that needs to happen but this is exactly as planned so really thank you.

@youknowriad youknowriad force-pushed the try/paragraph-line-height-control branch from 1aea158 to 777f079 Compare March 27, 2020 15:11
@youknowriad
Copy link
Contributor

I rebased this branch, for me aside potential test failures (wait for travis), this is ready.

@ItsJonQ
Copy link
Author

ItsJonQ commented Mar 27, 2020

Thank you so much @youknowriad !! 🤗
And to everyone for your amazing feedback and testing!

@ItsJonQ
Copy link
Author

ItsJonQ commented Mar 27, 2020

Oh boy!! Looks like Travis is green 🙌

Will merge it up!

@mwargan
Copy link

mwargan commented May 26, 2020

@ItsJonQ when disabling both font sizes and line height, I still see the Typography panel. Shouldn't this be removed?

@ItsJonQ
Copy link
Author

ItsJonQ commented May 26, 2020

@mwargan Ah yes! I submitted a PR here that fixes that case:
#22605

@afercia
Copy link
Contributor

afercia commented Jul 12, 2020

Late to the party. While I do realize there's sometimes the need to set custom line-heights, I also do share some of @ZebulanStanphill's concerns.

General considerations:

  1. Putting all this power in the users hands is potentially dangerous. Ensuring "true WYSIWYG" is one of the block editor goals but it shouldn't allow limitless customization power. There should be a limit to what users can and cannot do. Allowing users to indiscriminately play with font-sizes and line-heights potentially exposes to a complete disruption of the theme typography, potentially resulting in arguable results that will break readability of the front end content.
  2. The block editor should establish limits to the allowed line-height values. Too small values shouldn't be allowed as they harm readability.

Architectural considerations:

  1. The current implementation provides the ability to set only custom values. As such, the line-height value is saved as an inline style and permanently stored in the HTML blob in the database. This breaks the principle of separation of content from presentation. It also breaks interoperability of the data: exporting the content for reuse in other applications will bring all these presentational values in the content, where they would likely break the presentation. This brings us back to 15 years ago when web content was often an inextricable "HTML soup".
  2. Admittedly, this is not new to the line-height control. It already happens for custom font sizes etc. But, instead of moving away from inline styles, I see the block editor is keeping using inline styles. Maybe a bit too lightly. Instead of more inline styles, I'd expect Gutenberg to explore new solutions to make custom presentational values be stored separately from content. Note that the preset values use CSS classes insead, which is way better.
  3. As of July 2020, WordPress is powering 37.8% of the top 10 million websites in the world. This means the block editor is being polluting the content of 37.8% of the websites with presentational data. Not a great progress for the Web overall. I'd like the block editor team to be more aware of this and feel a stronger responsibility to make the Web better.

Implementation considerations:

  1. At the very least, I'd agree the line-height control should be paired with the font-size control and provide preset values in addition to the custom one. This would also encourage to use the preset values (CSS classes) instead of a custom value (inline style).
  2. A line-height value makes truly sense only in relationship with the font-size is used for. Only themes are ultimately responsible for the content font-size.
  3. When themes provide support for editor-font-sizes the should be required to pair a set of line-height values for each present font-size. Only this way, the line-height value makes sense because it's crafted on a specific font-size value. This would help consistency in the design and mitigate the "HTML soup" problem. Users can always set a custom value if they'r enot satisfied with the presets one.
  4. When themes do not provide support for editor-font-sizes, the block editor should provide sets of preset line-height values for each defaults in the fontSizes array. Again, only this way line-height values can be well-crafted in relationships to the actual font-size value.

Current user interface shortcomings:
I will create a separate, more detailed, issue. Also, in combination with some changes to the editor-styles-wrapper the preview in the block editor is not accurate. Some quick considerations (will better detail in the separate issue):

  1. Some themes use a default line-height with several decimal values. For example, Twenty Twenty uses 1.476 for the content paragraphs. The editor UI doesn't allow to set this value other than manually. The 0.1 step value for the number input type doesn't seem appropriate. I wonder whether a number input is appropriate in the first place but I can't think of of a better control off the top of my head.
  2. There's no clear way to reset a custom line-height value. The Backspace key works but that's hardly discoverable. At the very least, there should be a "Reset" button like for other custom values inputs.
  3. The placeholder value of 1.5 is highly confusing. At first sight, it seems the default value, while the actual value from the theme may be completely different. Not sure there's a reason to display the placeholder in the first place and I'd tend to think it should be removed. Noting that the font-size custom value doesn't use a placeholder, for good reasons.

line height control

Copy link

@kryslynn230 kryslynn230 left a comment

Choose a reason for hiding this comment

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

Approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Paragraph Affects the Paragraph Block [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Package] Block editor /packages/block-editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.