Global Styles: Display font families from theme, core, and user in font family picker#33889
Closed
jeyip wants to merge 2 commits into
Closed
Global Styles: Display font families from theme, core, and user in font family picker#33889jeyip wants to merge 2 commits into
jeyip wants to merge 2 commits into
Conversation
|
Size Change: -10.6 kB (-1%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
Mamaduka
reviewed
Aug 5, 2021
Comment on lines
+83
to
+87
| const fontFamilies = mergeWithoutDuplicateFontFamilies( | ||
| themeFontFamilies, | ||
| userFontFamilies, | ||
| coreFontFamilies | ||
| ); |
Member
There was a problem hiding this comment.
Nit: I think we can use _.unionBy(themeFontFamilies, userFontFamilies, coreFontFamilies, 'slug'); here.
jeyip
requested review from
Addison-Stavlo,
Copons,
carolinan,
creativecoder,
mattwiebe and
youknowriad
3 tasks
Member
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? |
Contributor
@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. 🤔 |
Contributor
Author
|
👋 Friendly ping @nosolosw. Wondering if you have thoughts on Addie's comment. |
Member
|
In terms of design, it'd be best to ask what design folks think of that approach. |
annezazu
added
the
Global Styles
Contributor
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:
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 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 |
|
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. |
Member
|
The target components for this PR have been heavily refactored, and changes are no longer applicable without a full rewrite. IIRC, a similar issue has been resolved via #61915. I'm going to close the PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
userfont families orthemefont families orcorefont families in the picker. If font families from multiple origins exist, the priority isuser > theme > core, and again, only one set of font families is made available.gutenberg/packages/edit-site/src/components/editor/utils.js
Lines 108 to 110 in 27942d7
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
themeorigin 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:
*.native.jsfiles for terms that need renaming or removal).