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.
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
Load variation font faces with theme relative urls using backend provided full urls (_links) #65019
Load variation font faces with theme relative urls using backend provided full urls (_links) #65019
Changes from 6 commits
6219a6d
1e8ef55
65f3754
77f7638
0bed01d
48bfe65
65d147a
c6a32f9
5a9e2d0
27b5e64
463fa95
9bd3378
5cecd18
c262896
8ced871
ee98b6e
1461989
8172039
7cb4c01
20a00e3
f2ad8e7
5a3b172
db89bf6
32868cd
efd9562
905f0d8
e85076a
a1cdf2c
2250a0c
59ce5a4
d94e2f2
607740a
b69ae26
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's hard to figure that out on your own but If I'm not wrong
useGlobalSetting
doesn't work consistently in all editors. In fact, we should probably avoid using it in the "block-editor" package entirely because it relies on the presence of a provider that may or may not exist (it doesn't exist in the post editor for instance).@aaronrobertshaw added a recently a new block editor setting that allows us to access global styles in all editors if I'm not wrong and we should probably be using that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ping 👍
This was specifically related to the styles data from Global Styles as the settings values were already included within the block editor settings.
The Global Styles settings and styles weren't merged as there wasn't consensus on what the shape of those settings should be while maintaining BC. That led to using a symbol as a private key for the Global Styles style data.
Here's the PR adding the style data to the block editor settings: #61556.
Without looking into the code I'm not sure whether the Global Styles settings under
__experimentalFeatures
in the editor settings are kept in sync with unsaved Global Styles changes in the site editor. It could be possible that aspect needs addressing or perhaps in the short term this could fallback to the editor settings if the Global Styles context isn't available (in post editor).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I mistakenly thought this is about the styles.
This PR reminds of the "styles" prop that we pass to the block editor to render the current theme styles. I could suggest a new "type" within the same "styles" prop to load fonts (like we do for svg for instance) but first. I would like to understand more.
What fonts are we trying to resolve here. I'm guessing the fonts that are used in the theme styles right? In which case, I feel like "resolving them" or "rendering them" should be done in the EditorStyles component. It's a component that is already used to "resolve or render" SVGs and CSS, it's already rendered in both iframe and non-iframe, so I'm starting to think that it's probably the right place for this change? (Introduce a new "type" of style and render it within that component)
Please correct me if my argumentation or abstraction seem too far off
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to access global styles settings in a component like EditorStyles without using
useGlobalSetting
. For this particular implementation I'm interested in getting the 'client side latest version' ofsettings.typography.fontFamilies
.@aaronrobertshaw could you show me how is this done? I haven't found how to access the fontFamilies path mentioned before.
Yes. This PR was motivated by that, but implementing this would make us able to remove the manual loading of fonts when a font is installed using the font library, too, so we could simplify a little bit that code.
@youknowriad I'm not sure about that, EditorStyles gets styles and not settings. At least, as it is today, that component is not getting the data needed to implement the desired functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EditorStyles renders the CSS variables for the color presets for instance, renders the SVG filters used by Duotone. Isn't it the same for fonts here? We're resolving the fonts that are used in the CSS.
It is possible that I'm misunderstanding the issue that this PR solves though and if it's the case please clarify? I'm assuming that resolving the fonts is done in the frontend somehow right? How is it done today?
The styles prop received by the
EditorStyles
component can contain things like thatThis represents all what's necessary for the "styles" to be rendered properly and to work properly.
Would it be out of the question to consider "fonts" at the same level as these and just add
Again, it's possible that I'm wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, right now that "styles" prop come from the block editor settings array that is passed from the backend to the frontend during the initialization of the editor. So custom CSS files are resolved there for instance and added to that array of
styles
In the site editor, since we have the ability to update global styles, that "styles" setting (the one that comes from backend to server) is also updated on the fly as we make changes to the global styles Here
gutenberg/packages/edit-site/src/components/global-styles-renderer/index.js
Line 38 in e1ad8c2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the
block-editor
store, undersettings.__experimentalFeatures.typography.fontFamilies
, you should find the data you're chasing in the post editor.One example of accessing theme.json settings via the block editor store can be found in the layout block support. Another example (that might change soon) is the block style variations block support which prefers the GlobalStyleContext data, if available, falling back to the block editor store's data.
Hope that helps a little 🤞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the insights.
In this commit I removed the use
useGlobalSetting
and replaced it by the use data from block-editor store and move the call to the font faces resolved inside the EditorStyles component: c6a32f9