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

Code block: Add support for font sizes #27294

Merged
merged 1 commit into from
Nov 26, 2020
Merged

Conversation

scruffian
Copy link
Contributor

Description

This adds support for font sizes to the code block. It also changes the CSS for this block to use the global styles variable with the old size as a fallback.

How has this been tested?

Tested with a block based and non block based themes (twentytwentyone)

Screenshots

Screenshot 2020-11-26 at 11 54 54

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.

@scruffian scruffian added [Block] Code Affects the Code Block [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Feature] Full Site Editing labels Nov 26, 2020
@scruffian scruffian self-assigned this Nov 26, 2020
@aristath
Copy link
Member

Hmmm I can see why we'd want that as an option in the block's global-styles, but having it available on a per-block basis seems like overkill to me. Though I don't think it can be available in global-styles without being exposed in the block options so I'm on the fence here 🤔

Code-wise this PR looks OK, it's the concept of the PR that troubles me a bit.

@github-actions
Copy link

Size Change: +69 B (0%)

Total Size: 1.2 MB

Filename Size Change
build/block-library/index.js 148 kB +2 B (0%)
build/block-library/style-rtl.css 8.26 kB +36 B (0%)
build/block-library/style.css 8.26 kB +37 B (0%)
build/block-library/theme-rtl.css 789 B -3 B (0%)
build/block-library/theme.css 790 B -3 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.8 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 664 B 0 B
build/block-directory/index.js 8.72 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 134 kB 0 B
build/block-editor/style-rtl.css 11.3 kB 0 B
build/block-editor/style.css 11.3 kB 0 B
build/block-library/editor-rtl.css 8.95 kB 0 B
build/block-library/editor.css 8.96 kB 0 B
build/block-serialization-default-parser/index.js 1.87 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 172 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/compose/index.js 9.95 kB 0 B
build/core-data/index.js 14.8 kB 0 B
build/data-controls/index.js 827 B 0 B
build/data/index.js 8.8 kB 0 B
build/date/index.js 11.2 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.95 kB 0 B
build/edit-navigation/index.js 11.1 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.42 kB 0 B
build/edit-post/style.css 6.4 kB 0 B
build/edit-site/index.js 24 kB 0 B
build/edit-site/style-rtl.css 3.92 kB 0 B
build/edit-site/style.css 3.92 kB 0 B
build/edit-widgets/index.js 26.3 kB 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/editor/index.js 43.3 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.62 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 6.86 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.27 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 698 B 0 B
build/keyboard-shortcuts/index.js 2.54 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.1 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/notices/index.js 1.81 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/reusable-blocks/index.js 2.92 kB 0 B
build/rich-text/index.js 13.3 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.05 kB 0 B
build/viewport/index.js 1.86 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@jasmussen
Copy link
Contributor

In general my hope for global styles is that it can provide solid defaults for these things, which can then be overridden on a per-block basis. That appears to be the case with this PR. 👍

It then becomes a question of whether a feature should then be highlighted as something to override on a per-block basis. That's what Ari refers to — is it a good user experience to surface font size for the code block? On the one hand, if the code size is too small, ideally you should change the font size for all code blocks, in Global Styles, not just one. On the other hand, I can see a use case — say for highlighting some code in a blog post extra large — where you'd want a default font size, and extra emphasis for just this one block.

I think there's a separate opportunity for us to improve the design of sidebar components to balance the prominence of these features, that perhaps also provide a shortcut to editing the global styes instance. But in the mean time, I think I'm leaning towards this one being cool, because of the use case mentioned.

@oandregal
Copy link
Member

I think this highlights a conversation we're having at the moment: how to control what style properties are exposed at the block level and at the global level #27295

Copy link
Member

@aristath aristath left a comment

Choose a reason for hiding this comment

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

After reading all the comments above (thank you for your points of view, it helps) and spending some time thinking about this, I can see the benefit in having the font-size control. 👍

@scruffian scruffian merged commit 8b748e3 into master Nov 26, 2020
@scruffian scruffian deleted the update/code-block-font-size branch November 26, 2020 20:35
@github-actions github-actions bot added this to the Gutenberg 9.5 milestone Nov 26, 2020
@scruffian
Copy link
Contributor Author

Thanks @aristath

white-space: pre-wrap;
overflow-wrap: break-word;
.wp-block-code {
font-size: var(--wp--preset--font-size--extra-small, 0.9em);
Copy link
Member

Choose a reason for hiding this comment

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

👋 we shouldn't have used a CSS Custom Property here, explained the rationale, and prepared a bugfix at #27672

@oandregal oandregal added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Dec 23, 2020
@oandregal
Copy link
Member

I've added the "Needs Dev Note" in this issue so we don't forget to create a Dev Note for WP 5.7 for this kind of change: list all the new style properties that were added to blocks and users can update. This should be useful for themes and plugins to check whether they need to adapt anything. Hat tip to Riad for the suggestion.

@noisysocks noisysocks mentioned this pull request Jan 28, 2021
9 tasks
@oandregal
Copy link
Member

As per #28539 (comment) I've removed the "Needs dev note" from this PR.

@oandregal oandregal removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Code Affects the Code Block [Feature] Themes Questions or issues with incorporating or styling blocks in a theme.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants