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

Preformatted block: Add support for font sizes #27584

Merged
merged 8 commits into from
Dec 15, 2020
Merged

Preformatted block: Add support for font sizes #27584

merged 8 commits into from
Dec 15, 2020

Conversation

mendezcode
Copy link
Contributor

Description

Adds support for font sizes to the preformatted code block. It also changes the CSS for this block to use the global styles variable, setting a fallback font size.

How has this been tested?

Tested with a block based and non block based version of twentytwentyone.

Screenshots

Screen Shot 2020-12-08 at 12 59 17 PM

Types of changes

New feature (non-breaking change which adds functionality).

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.

@@ -0,0 +1,4 @@
.wp-block-preformatted {
overflow-x: auto;
Copy link
Contributor

Choose a reason for hiding this comment

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

The style.scss file always gets loaded into the editor, so if a rule exists in that file, it does not need to be replicated in editor.scss.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yes, true that. I'll fix accordingly.

@@ -0,0 +1,4 @@
.wp-block-preformatted {
overflow-x: auto;
white-space: pre !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this rule also exists in style.scss, we are so close to not needing any editor style at all for this block. And we should be avoiding !important anyway, if at all possible. I dug in to see why the !important was necessary, and it appears that white-space: pre-wrap; is applied as an inline style on the preformatted block:

Screenshot 2020-12-09 at 08 20 30

Why does that exist? I'm not sure, but from digging at the code, it looks to be related to this PR: #18656. Or possibly #17779. Specifically there's some commentary in https://github.com/WordPress/gutenberg/pull/17779/files#diff-7df6f5253ccec2f2e6d1bb22e81f87a4b6bccd2604463eeecded163aa27fd333R70 which seems relevant.

It seems we need to figure out why that output

Copy link
Contributor Author

@mendezcode mendezcode Dec 14, 2020

Choose a reason for hiding this comment

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

The pre-wrap inline style is coming from the <RichText> component, and its value is defined here: https://github.com/WordPress/gutenberg/blob/master/packages/rich-text/src/component/index.js#L82

The value is set on the whiteSpace variable, which is then added to defaultStyle, which subsequently makes it to the component's inline styles via the style prop here: https://github.com/WordPress/gutenberg/blob/master/packages/rich-text/src/component/index.js#L1075

According to the inspector, I can see that besides the pre-wrap inline style, there's the pre-wrap style attribute coming from the Block's style.scss stylesheet. If neither the inline style nor the block style are present, then, the <pre> element will get its default style from the browser defaults.

This makes me think we can live with the inline style, since the <RichText> component is using the style attribute to guarantee its proper display (my assumption). Assuming a future scenario where the <RichText> component removes the pre-wrap inline style, then we'll still be covered by the pre-wrap style available in the preformatted block's style.scss.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just remove the inline style?

Copy link
Contributor Author

@mendezcode mendezcode Dec 15, 2020

Choose a reason for hiding this comment

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

That's a quite considerable change. The <RichText> component uses it by default, and it (the component) is used in many places. Changing this may change the overall behavior of the <RichText> component.

@jasmussen
Copy link
Contributor

Thanks so much for this PR! Here's the editor, before:

editor before

Frontend, before:

front before

Here's the editor, after:

editor after

Here's the frontend, after:

front after

The font size control is also added:

Screenshot 2020-12-09 at 08 26 23

That font size stuff all seems fine. It also adds a white-space: nowrap; property with a horizontal scrollbar. While it's good that this makes the frontend and editor sync up, this is the part of the PR we need to tread carefully with:

  1. It changes the default appearance of the block which is already in wide usage. While I generally think it's an okay change, it is nevertheless a change, which means we need to tread carefully.
  2. As I also left some comments about, it changes the intrinsic pre-wrap property to wrap, which based on commentary in the code by @ellatrix might go against initial intentions.

It seems the safest way forward, which still puts the frontend in sync with the editor, is to go in the opposite direction.

Instead of:

.wp-block-preformatted {
	font-size: var(--wp--preset--font-size--extra-small, 1em);
	overflow-x: auto;
	white-space: pre;
}

you output

.wp-block-preformatted {
	font-size: var(--wp--preset--font-size--extra-small, 1em);
	white-space: pre-wrap;
}

That way you can also remove the editor style you added again, and we sync up the frontend.

I'd also argue that this serves as a good bugfix for the lacklustre overflow handling.

What do you think?

This is a more elegant solution compared to the previous one, which relied on overflow.
@mendezcode
Copy link
Contributor Author

mendezcode commented Dec 9, 2020

It changes the default appearance of the block which is already in wide usage. While I generally think it's an okay change, it is nevertheless a change, which means we need to tread carefully.

@jasmussen What I think would make sense here is to do the following:

Set the default font size to match the existing size in Gutenberg. Right now it is --wp--preset--font-size--extra-small, and it definitely looks different compared to the size before. It may need to be set to the current value it has at the moment. I haven't looked, but there may be a bigger font size that matches the current size of the editor.

My thinking here is that this needs to be done as progressive enhancement instead of doing it like I did initially. It must have backwards compatibility with the existing preformatted block, which means it would be totally better for it to have the same size it had.

I'll work on the backwards compatibility fix for the font size and will keep you posted.

What do you think?

Makes absolute sense, I pushed a commit to address the pre-wrap change. Thanks!

@jasmussen
Copy link
Contributor

Set the default font size to match the existing size in Gutenberg. Right now it is --wp--preset--font-size--extra-small, and it definitely looks different compared to the size before. It may need to be set to the current value it has at the moment. I haven't looked, but there may be a bigger font size that matches the current size of the editor.

The size looks the same to me in my testing of TT1🔳s, but definitely worth testing the before and after in a few themes to be sure.

My thinking here is that this needs to be done as progressive enhancement instead of doing it like I did initially. It must have backwards compatibility with the existing preformatted block, which means it would be totally better for it to have the same size it had.

I think that's generally the safe mindset, but it doesn't have to be the case always. If the existing block has a bug or something in that category, it's fine to fix it. So the fact that it goes out to every user, is arguably a key benefit. So this isn't a fundamental issue, so much as us being careful that we do this deliberately and thinking through any potential side effects.

@mendezcode
Copy link
Contributor Author

I have removed editor.scss to leave only the pre-wrap coming from style.scss.
This does align with the pre-wrap present in the editor inline style.

@@ -0,0 +1,4 @@
.wp-block-preformatted {
font-size: var(--wp--preset--font-size--extra-small, 1em);
Copy link
Member

Choose a reason for hiding this comment

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

👋 We should remove the CSS Custom Property from here. I've seen that the code block recently got it added as well in https://github.com/WordPress/gutenberg/pull/27294/files#r540890153 and I've prepared a PR that fixes and explain why we shouldn't do that #27672

@oandregal
Copy link
Member

The changes to add the font-size look fine by me (other than removing the CSS Custom Property, see comment). I see there's some conversation about how to handle the wrapping, as soon as that's sorted out, it's fine to land this PR.

This is the theme's job. For more information, see #27672
@jasmussen jasmussen self-requested a review December 15, 2020 07:52
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

This seems good to me! I'd love a sanity check from Andrés or Ben or Kjell, but otherwise, thank you for your work and congrats on your first PR!

Copy link
Contributor

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

LGTM

@scruffian
Copy link
Contributor

We should also add this to the docs (docs/designers-developers/developers/themes/theme-json.md)

mendezcode and others added 2 commits December 15, 2020 08:18
This is added due to new requirement implemented in #25220

Co-authored-by: Ari Stathopoulos <[email protected]>
@scruffian scruffian merged commit cbbc353 into WordPress:master Dec 15, 2020
@github-actions
Copy link

Congratulations on your first merged pull request, @mendezcode! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants