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

Global Styles: Display font families from theme, core, and user in font family picker #33889

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

jeyip
Copy link
Contributor

@jeyip jeyip commented Aug 5, 2021

Description

I'm attempting to extend global styles by adding more font family options to the global styles font family picker, and am running into issues with how global styles "merges" data from different origins.

The current behavior displays user font families or theme font families or core font families in the picker. If font families from multiple origins exist, the priority is user > theme > core, and again, only one set of font families is made available.

if ( result && PATHS_WITH_MERGE[ path ] ) {
return result.user ?? result.theme ?? result.core;
}

This makes extending fonts tricky, because we have to determine which origins are present and to merge values into the correct origin, otherwise they're not displayed. Semantically, we also have an odd situation where we're having to extend font options by adding to the theme origin settings to have them appear in the picker, even though the new fonts are not theme related.

I was curious what we think about merging font families from all origins, which is what I've done in this PR.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • 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 (please manually search all *.native.js files for terms that need renaming or removal).

@jeyip jeyip self-assigned this Aug 5, 2021
@github-actions
Copy link

github-actions bot commented Aug 5, 2021

Size Change: -10.6 kB (-1%)

Total Size: 1.07 MB

Filename Size Change
build/annotations/index.min.js 2.93 kB +2 B (0%)
build/block-directory/index.min.js 6.62 kB +1 B (0%)
build/block-editor/index.min.js 127 kB -21 B (0%)
build/block-library/index.min.js 147 kB +3 B (0%)
build/blocks/index.min.js 47.2 kB -1 B (0%)
build/components/index.min.js 216 kB +18.8 kB (+10%) ⚠️
build/compose/index.min.js 10.3 kB +56 B (+1%)
build/data/index.min.js 7.22 kB +1 B (0%)
build/deprecated/index.min.js 737 B -1 B (0%)
build/dom/index.min.js 4.78 kB +1 B (0%)
build/edit-navigation/index.min.js 13.9 kB +1 B (0%)
build/edit-post/index.min.js 29.9 kB -29.5 kB (-50%) 🏆
build/edit-post/style-rtl.css 7.18 kB -13 B (0%)
build/edit-post/style.css 7.17 kB -12 B (0%)
build/edit-site/index.min.js 26.1 kB +78 B (0%)
build/edit-widgets/index.min.js 16.6 kB -2 B (0%)
build/editor/index.min.js 38.3 kB -3 B (0%)
build/element/index.min.js 3.44 kB -1 B (0%)
build/list-reusable-blocks/index.min.js 2.07 kB +1 B (0%)
build/notices/index.min.js 1.07 kB +1 B (0%)
build/nux/index.min.js 2.31 kB +2 B (0%)
build/react-i18n/index.min.js 922 B -2 B (0%)
build/redux-routine/index.min.js 2.82 kB +2 B (0%)
build/token-list/index.min.js 847 B -1 B (0%)
build/url/index.min.js 1.95 kB +1 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 1.12 kB
build/admin-manifest/index.min.js 1.46 kB
build/api-fetch/index.min.js 2.44 kB
build/autop/index.min.js 2.28 kB
build/blob/index.min.js 673 B
build/block-directory/style-rtl.css 1.01 kB
build/block-directory/style.css 1.01 kB
build/block-editor/style-rtl.css 13.9 kB
build/block-editor/style.css 13.8 kB
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 65 B
build/block-library/blocks/archives/style.css 65 B
build/block-library/blocks/audio/editor-rtl.css 58 B
build/block-library/blocks/audio/editor.css 58 B
build/block-library/blocks/audio/style-rtl.css 111 B
build/block-library/blocks/audio/style.css 111 B
build/block-library/blocks/audio/theme-rtl.css 125 B
build/block-library/blocks/audio/theme.css 125 B
build/block-library/blocks/block/editor-rtl.css 161 B
build/block-library/blocks/block/editor.css 161 B
build/block-library/blocks/button/editor-rtl.css 474 B
build/block-library/blocks/button/editor.css 474 B
build/block-library/blocks/button/style-rtl.css 605 B
build/block-library/blocks/button/style.css 604 B
build/block-library/blocks/buttons/editor-rtl.css 315 B
build/block-library/blocks/buttons/editor.css 315 B
build/block-library/blocks/buttons/style-rtl.css 370 B
build/block-library/blocks/buttons/style.css 370 B
build/block-library/blocks/calendar/style-rtl.css 207 B
build/block-library/blocks/calendar/style.css 207 B
build/block-library/blocks/categories/editor-rtl.css 84 B
build/block-library/blocks/categories/editor.css 83 B
build/block-library/blocks/categories/style-rtl.css 79 B
build/block-library/blocks/categories/style.css 79 B
build/block-library/blocks/code/style-rtl.css 90 B
build/block-library/blocks/code/style.css 90 B
build/block-library/blocks/code/theme-rtl.css 131 B
build/block-library/blocks/code/theme.css 131 B
build/block-library/blocks/columns/editor-rtl.css 189 B
build/block-library/blocks/columns/editor.css 188 B
build/block-library/blocks/columns/style-rtl.css 474 B
build/block-library/blocks/columns/style.css 475 B
build/block-library/blocks/cover/editor-rtl.css 666 B
build/block-library/blocks/cover/editor.css 670 B
build/block-library/blocks/cover/style-rtl.css 1.23 kB
build/block-library/blocks/cover/style.css 1.23 kB
build/block-library/blocks/embed/editor-rtl.css 488 B
build/block-library/blocks/embed/editor.css 488 B
build/block-library/blocks/embed/style-rtl.css 400 B
build/block-library/blocks/embed/style.css 400 B
build/block-library/blocks/embed/theme-rtl.css 124 B
build/block-library/blocks/embed/theme.css 124 B
build/block-library/blocks/file/editor-rtl.css 300 B
build/block-library/blocks/file/editor.css 300 B
build/block-library/blocks/file/style-rtl.css 255 B
build/block-library/blocks/file/style.css 255 B
build/block-library/blocks/file/view.min.js 711 B
build/block-library/blocks/freeform/editor-rtl.css 2.44 kB
build/block-library/blocks/freeform/editor.css 2.44 kB
build/block-library/blocks/gallery/editor-rtl.css 707 B
build/block-library/blocks/gallery/editor.css 706 B
build/block-library/blocks/gallery/style-rtl.css 1.05 kB
build/block-library/blocks/gallery/style.css 1.05 kB
build/block-library/blocks/gallery/theme-rtl.css 122 B
build/block-library/blocks/gallery/theme.css 122 B
build/block-library/blocks/group/editor-rtl.css 159 B
build/block-library/blocks/group/editor.css 159 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 93 B
build/block-library/blocks/group/theme.css 93 B
build/block-library/blocks/heading/editor-rtl.css 152 B
build/block-library/blocks/heading/editor.css 152 B
build/block-library/blocks/heading/style-rtl.css 76 B
build/block-library/blocks/heading/style.css 76 B
build/block-library/blocks/home-link/style-rtl.css 247 B
build/block-library/blocks/home-link/style.css 247 B
build/block-library/blocks/html/editor-rtl.css 283 B
build/block-library/blocks/html/editor.css 284 B
build/block-library/blocks/image/editor-rtl.css 728 B
build/block-library/blocks/image/editor.css 728 B
build/block-library/blocks/image/style-rtl.css 482 B
build/block-library/blocks/image/style.css 487 B
build/block-library/blocks/image/theme-rtl.css 124 B
build/block-library/blocks/image/theme.css 124 B
build/block-library/blocks/latest-comments/style-rtl.css 284 B
build/block-library/blocks/latest-comments/style.css 284 B
build/block-library/blocks/latest-posts/editor-rtl.css 137 B
build/block-library/blocks/latest-posts/editor.css 137 B
build/block-library/blocks/latest-posts/style-rtl.css 528 B
build/block-library/blocks/latest-posts/style.css 527 B
build/block-library/blocks/list/style-rtl.css 63 B
build/block-library/blocks/list/style.css 63 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 488 B
build/block-library/blocks/media-text/style.css 485 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 474 B
build/block-library/blocks/navigation-link/editor.css 474 B
build/block-library/blocks/navigation-link/style-rtl.css 94 B
build/block-library/blocks/navigation-link/style.css 94 B
build/block-library/blocks/navigation/editor-rtl.css 1.67 kB
build/block-library/blocks/navigation/editor.css 1.68 kB
build/block-library/blocks/navigation/style-rtl.css 1.65 kB
build/block-library/blocks/navigation/style.css 1.64 kB
build/block-library/blocks/navigation/view.min.js 2.84 kB
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 310 B
build/block-library/blocks/page-list/editor.css 310 B
build/block-library/blocks/page-list/style-rtl.css 242 B
build/block-library/blocks/page-list/style.css 242 B
build/block-library/blocks/paragraph/editor-rtl.css 157 B
build/block-library/blocks/paragraph/editor.css 157 B
build/block-library/blocks/paragraph/style-rtl.css 248 B
build/block-library/blocks/paragraph/style.css 248 B
build/block-library/blocks/post-author/editor-rtl.css 210 B
build/block-library/blocks/post-author/editor.css 210 B
build/block-library/blocks/post-author/style-rtl.css 182 B
build/block-library/blocks/post-author/style.css 181 B
build/block-library/blocks/post-comments-form/style-rtl.css 140 B
build/block-library/blocks/post-comments-form/style.css 140 B
build/block-library/blocks/post-comments/style-rtl.css 360 B
build/block-library/blocks/post-comments/style.css 359 B
build/block-library/blocks/post-content/editor-rtl.css 138 B
build/block-library/blocks/post-content/editor.css 138 B
build/block-library/blocks/post-excerpt/editor-rtl.css 73 B
build/block-library/blocks/post-excerpt/editor.css 73 B
build/block-library/blocks/post-excerpt/style-rtl.css 69 B
build/block-library/blocks/post-excerpt/style.css 69 B
build/block-library/blocks/post-featured-image/editor-rtl.css 412 B
build/block-library/blocks/post-featured-image/editor.css 412 B
build/block-library/blocks/post-featured-image/style-rtl.css 143 B
build/block-library/blocks/post-featured-image/style.css 143 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-template/style-rtl.css 378 B
build/block-library/blocks/post-template/style.css 379 B
build/block-library/blocks/post-terms/style-rtl.css 73 B
build/block-library/blocks/post-terms/style.css 73 B
build/block-library/blocks/post-title/style-rtl.css 60 B
build/block-library/blocks/post-title/style.css 60 B
build/block-library/blocks/preformatted/style-rtl.css 103 B
build/block-library/blocks/preformatted/style.css 103 B
build/block-library/blocks/pullquote/editor-rtl.css 198 B
build/block-library/blocks/pullquote/editor.css 198 B
build/block-library/blocks/pullquote/style-rtl.css 361 B
build/block-library/blocks/pullquote/style.css 360 B
build/block-library/blocks/pullquote/theme-rtl.css 167 B
build/block-library/blocks/pullquote/theme.css 167 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 270 B
build/block-library/blocks/query-pagination/editor.css 262 B
build/block-library/blocks/query-pagination/style-rtl.css 168 B
build/block-library/blocks/query-pagination/style.css 168 B
build/block-library/blocks/query-title/editor-rtl.css 85 B
build/block-library/blocks/query-title/editor.css 85 B
build/block-library/blocks/query/editor-rtl.css 131 B
build/block-library/blocks/query/editor.css 132 B
build/block-library/blocks/quote/style-rtl.css 169 B
build/block-library/blocks/quote/style.css 169 B
build/block-library/blocks/quote/theme-rtl.css 220 B
build/block-library/blocks/quote/theme.css 222 B
build/block-library/blocks/rss/editor-rtl.css 202 B
build/block-library/blocks/rss/editor.css 204 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 209 B
build/block-library/blocks/search/editor.css 209 B
build/block-library/blocks/search/style-rtl.css 368 B
build/block-library/blocks/search/style.css 372 B
build/block-library/blocks/search/theme-rtl.css 64 B
build/block-library/blocks/search/theme.css 64 B
build/block-library/blocks/separator/editor-rtl.css 99 B
build/block-library/blocks/separator/editor.css 99 B
build/block-library/blocks/separator/style-rtl.css 250 B
build/block-library/blocks/separator/style.css 250 B
build/block-library/blocks/separator/theme-rtl.css 172 B
build/block-library/blocks/separator/theme.css 172 B
build/block-library/blocks/shortcode/editor-rtl.css 474 B
build/block-library/blocks/shortcode/editor.css 474 B
build/block-library/blocks/site-logo/editor-rtl.css 462 B
build/block-library/blocks/site-logo/editor.css 464 B
build/block-library/blocks/site-logo/style-rtl.css 153 B
build/block-library/blocks/site-logo/style.css 153 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 84 B
build/block-library/blocks/site-title/editor.css 84 B
build/block-library/blocks/social-link/editor-rtl.css 165 B
build/block-library/blocks/social-link/editor.css 165 B
build/block-library/blocks/social-links/editor-rtl.css 812 B
build/block-library/blocks/social-links/editor.css 811 B
build/block-library/blocks/social-links/style-rtl.css 1.33 kB
build/block-library/blocks/social-links/style.css 1.33 kB
build/block-library/blocks/spacer/editor-rtl.css 307 B
build/block-library/blocks/spacer/editor.css 307 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 471 B
build/block-library/blocks/table/editor.css 472 B
build/block-library/blocks/table/style-rtl.css 481 B
build/block-library/blocks/table/style.css 481 B
build/block-library/blocks/table/theme-rtl.css 188 B
build/block-library/blocks/table/theme.css 188 B
build/block-library/blocks/tag-cloud/style-rtl.css 146 B
build/block-library/blocks/tag-cloud/style.css 146 B
build/block-library/blocks/template-part/editor-rtl.css 636 B
build/block-library/blocks/template-part/editor.css 635 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/term-description/editor-rtl.css 90 B
build/block-library/blocks/term-description/editor.css 90 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 87 B
build/block-library/blocks/verse/style.css 87 B
build/block-library/blocks/video/editor-rtl.css 571 B
build/block-library/blocks/video/editor.css 572 B
build/block-library/blocks/video/style-rtl.css 173 B
build/block-library/blocks/video/style.css 173 B
build/block-library/blocks/video/theme-rtl.css 124 B
build/block-library/blocks/video/theme.css 124 B
build/block-library/common-rtl.css 1.29 kB
build/block-library/common.css 1.29 kB
build/block-library/editor-rtl.css 9.87 kB
build/block-library/editor.css 9.85 kB
build/block-library/reset-rtl.css 514 B
build/block-library/reset.css 514 B
build/block-library/style-rtl.css 10.2 kB
build/block-library/style.css 10.2 kB
build/block-library/theme-rtl.css 688 B
build/block-library/theme.css 692 B
build/block-serialization-default-parser/index.min.js 1.3 kB
build/block-serialization-spec-parser/index.min.js 3.06 kB
build/components/style-rtl.css 15.8 kB
build/components/style.css 15.8 kB
build/core-data/index.min.js 12.6 kB
build/customize-widgets/index.min.js 10.8 kB
build/customize-widgets/style-rtl.css 1.5 kB
build/customize-widgets/style.css 1.49 kB
build/data-controls/index.min.js 831 B
build/date/index.min.js 31.8 kB
build/dom-ready/index.min.js 576 B
build/edit-navigation/style-rtl.css 3.1 kB
build/edit-navigation/style.css 3.1 kB
build/edit-post/classic-rtl.css 479 B
build/edit-post/classic.css 481 B
build/edit-site/style-rtl.css 5.01 kB
build/edit-site/style.css 5.01 kB
build/edit-widgets/style-rtl.css 4.01 kB
build/edit-widgets/style.css 4.02 kB
build/editor/style-rtl.css 3.92 kB
build/editor/style.css 3.91 kB
build/escape-html/index.min.js 739 B
build/format-library/index.min.js 5.72 kB
build/format-library/style-rtl.css 668 B
build/format-library/style.css 669 B
build/hooks/index.min.js 1.76 kB
build/html-entities/index.min.js 628 B
build/i18n/index.min.js 3.73 kB
build/is-shallow-equal/index.min.js 710 B
build/keyboard-shortcuts/index.min.js 1.74 kB
build/keycodes/index.min.js 1.49 kB
build/list-reusable-blocks/style-rtl.css 838 B
build/list-reusable-blocks/style.css 838 B
build/media-utils/index.min.js 3.08 kB
build/nux/style-rtl.css 747 B
build/nux/style.css 743 B
build/plugins/index.min.js 1.99 kB
build/primitives/index.min.js 1.06 kB
build/priority-queue/index.min.js 791 B
build/reusable-blocks/index.min.js 2.56 kB
build/reusable-blocks/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/rich-text/index.min.js 10.8 kB
build/server-side-render/index.min.js 1.64 kB
build/shortcode/index.min.js 1.68 kB
build/viewport/index.min.js 1.28 kB
build/warning/index.min.js 1.16 kB
build/widgets/index.min.js 6.48 kB
build/widgets/style-rtl.css 1.04 kB
build/widgets/style.css 1.04 kB
build/wordcount/index.min.js 1.24 kB

compressed-size-action

Comment on lines +83 to +87
const fontFamilies = mergeWithoutDuplicateFontFamilies(
themeFontFamilies,
userFontFamilies,
coreFontFamilies
);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think we can use _.unionBy(themeFontFamilies, userFontFamilies, coreFontFamilies, 'slug'); here.

@jeyip jeyip marked this pull request as draft August 5, 2021 05:21
@oandregal
Copy link
Member

I'm attempting to extend global styles by adding more font family options to the global styles font family picker, and am running into issues with how global styles "merges" data from different origins. (....) I was curious what we think about merging font families from all origins.

Merging all font families is not the expected behavior as of now, so I wouldn't feel comfortable adding it without designer approval.

What are the issues with extending the origin of data that has the highest priority? For example: detecting that theme has provided some fonts and add yours there, otherwise, add them to the core origin. Would something like that work for you?

@Addison-Stavlo
Copy link
Contributor

Addison-Stavlo commented Aug 6, 2021

What are the issues with extending the origin of data that has the highest priority? For example: detecting that theme has provided some fonts and add yours there, otherwise, add them to the core origin. Would something like that work for you?

@nosolosw - I think that could be a solution, although we wanted to explore if there was a more clear way to extend the list first. The idea being that a theme supplies specific fonts for a reason: they fit the design of the theme / are the fonts the theme recommends. In extending the list it would be nice to keep a distinction between the fonts recommended by the theme designers and the more general additions, so that the 'most fitting' fonts are not lost in the much longer list of general font options that would be available regardless of theme. 🤔

@jeyip
Copy link
Contributor Author

jeyip commented Aug 11, 2021

👋 Friendly ping @nosolosw. Wondering if you have thoughts on Addie's comment.

@oandregal
Copy link
Member

In terms of design, it'd be best to ask what design folks think of that approach.

@annezazu annezazu added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Aug 12, 2021
@carolinan carolinan added the Needs Design Feedback Needs general design feedback. label Aug 28, 2021
@jasmussen
Copy link
Contributor

Merging all font families is not the expected behavior as of now, so I wouldn't feel comfortable adding it without designer approval.

Happy to help think through this. What are the primary concerns? I'm almost certainly missing some nuance, but here's what I can picture:

  1. The list of font stacks becomes very long, combining both presets and theme additions.
  2. The list has a lot of font stacks that appear to be duplicate: one might have Helvetica, sans-serif and another might have Helvetica, Helvetica Neue, Arial, sans-serif, with no clear indication of the benefit of either choice.
  3. A user might choose a theme font-stack, then change themes, what happens then? (Same challenge as exists with palettes I suppose)

2 could be somewhat frustrating, but could potentially be addressed by showing all fonts dividied by subheadings and separators in the font picker dropdown. 1 can possibly be solved with design — a long list might not be overwhelming if the font title is easily glancable (we might want to emphasize the first font in the stack, and de-emphasize or hide the rest of the stack). But both of these suggest the dropdown could need smarts beyond what a vanilla select can offer, a la the font-size dropdown.

3 is likely the key challenge to solve, and mostly on a technical level. In principle, the use case of extending the list of available fonts beyond what core and themes provide feels legitimate.

CC: @pablohoneyhoney

@pablohoneyhoney
Copy link

Thanks for the ping @jasmussen

We'll be providing soon some updated designs in #34574 on how we imagine the font picker flows, and perhaps how we could present extensibility to the user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants