-
Notifications
You must be signed in to change notification settings - Fork 798
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
Add Google Fonts provider and register a selection of fonts #23045
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
|
d39d581
to
3faba63
Compare
projects/packages/google-fonts-provider/src/class-google-fonts-provider.php
Show resolved
Hide resolved
wpcom diff: D75501-code |
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.
Just made a few general comments. I think we could merge this early on and iterate later on, if you'd like to test this further.
projects/packages/google-fonts-provider/src/class-google-fonts-provider.php
Outdated
Show resolved
Hide resolved
* Recommendation Order: 2 | ||
* First Introduced: $$next-version$$ | ||
* Requires Connection: No | ||
* Auto Activate: Yes |
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 don't know that I would auto-activate the module until we have a UI for it in the Jetpack > Dashboard, so folks can disable the feature if they'd like it.
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.
Changed to No
. Do you know how we could auto-activate it only on Atomic sites?
Let me know if there are any other module header changes you would recommend. I wasn't sure how to set things like recommendation order.
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.
There are different ways we could go about it, depending on how you'd like to approach the deployment of this feature. Here is what comes to mind:
- If you'd rather not offer any UI, and enable it on all WoA sites right away, we could get rid of this module file in Jetpack, and instead require and instantiate the package from wpcomsh. This is what we do for the post-list package for example; the package isn't used in the Jetpack plugin, but is used in the wpcomsh plugin.
- If you'd like to offer a basic UI (in the legacy full module list) that would eventually turn into a proper setting in the Jetpack dashboard once the feature is deemed ready, you could enable the module on WoA like we do for the masterbar feature in wpcomsh. Check the
feature-plugins
directory for an example.
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 think it'd be nice to have a minimal UI for the module in Jetpack. I imagine lot's of plugin users will be interested in this feature, outside of WP.com hosted sites.
If you're willing, I'd like to get this basic version merged without a UI, and follow up with auto-activating the module for Atomic sites (wpcomsh) and adding a basic UI (Jetpack).
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.
That works for me!
Noting that the PR has now been merged. |
* Sort Order: 1 | ||
* Recommendation Order: 2 | ||
* First Introduced: $$next-version$$ | ||
* Requires Connection: No |
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.
@jeherve While the Google Fonts module doesn't require a Jetpack connection to work, do we need to put ithe module behind a connection to make sure users have agreed to Jetpack's terms, first? Loading Google fonts from a site does have some privacy implications for visitors to the site.
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.
That's a good question. I think those privacy implications may be between Google and the site owner, I'm not sure that's something we can cover in our own terms? That may be a better question to ask in +automattlock maybe?
Noting that D75501-code (for wpcom testing) is now updated with the latest module and provider changes from this PR. |
ea36271
to
27403b9
Compare
Our replacement tool doesn't support those headers yet.
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.
This tests well for me. Let's merge this, and iterate with a UI and maybe changes to address any legal requirements as we discussed.
Follow-up to #23045 This adds: - a new check to the initial state passed by PHP, to indicate whether a site can use the webfonts feature. - a new interface under Jetpack > Settings > Writing.
I've created a draft support doc we can update once we have some more information about the feature and its activation, and I've created #23295 to add a toggle to the Jetpack dashboard to turn the feature on and off. |
Follow-up to #23045 Let's favor using a Core implementation with a Core filter instead of manually adding to the head.
…n. (#23322) * Google Fonts: update method used to preconnect Fonts source domain Follow-up to #23045 Let's favor using a Core implementation with a Core filter instead of manually adding to the head. * Use filter instead of action * Only add resource hint if the site supports the Webfonts API See #23306 * Update readme with latest method name. * Introduce new class that doesn't depend on WP_Webfonts_Provider WP_Webfonts_Provider may not always be available, so we cannot call a method from a child class of `WP_Webfonts_Provider` from a WP filter. Let's introduce a new class instead, that doesn't depend on anything, and do the class check inside there. That should simplify things a bit for the folks having to implement that filtering. Co-authored-by: Brandon Kraft <[email protected]>
Changes proposed in this Pull Request:
Jetpack product discussion
paYE8P-1qo-p2
Does this pull request change what data or activity we track or use?
Loads Google Fonts settings and font files using Google APIs, if the module is activated.
Testing instructions:
Install the Gutenberg plugin with changes from this PR WordPress/gutenberg#37140 (now in Gutenberg
trunk
, but not yet released).pnpm jetpack install -r && pnpm jetpack install --all
to install the new fonts provider package./wp-admin/admin.php?page=jetpack_modules