Comment Likes: do not load on AMP views #14840
Merged
Conversation
This file contains 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
Summary: This patch disables enqueuing of assets for some non-functional features when in AMP mode. Noticons is a very large CSS asset, and so it prevents a lot of other CSS from being enqueued. A review of the places we use it showed that it was not actually being used by any functionality that works in AMP mode. Affected features: - Comment Likes (non-functional in AMP) - Geo Location (not used on most pages - might be useful to port `wpcom_vip_load_geolocation_styles_only_when_needed()` to all sites) - Notifications UI (non-functional in AMP) - admin-bar - (non-functional in AMP) Test Plan: Load a page in Transitional mode Ensure that noticons-css is not enqueued, and that little or no CSS is blocked by AMP due to being over the 50kb max [x] - ensure that features whose CSS is not being enqueued are not actually used Reviewers: josephscott, #devops_team, davidbinovec, goldsounds Subscribers: davidbinovec Tags: #touches_jetpack_files Differential Revision: D38340-code This commit syncs r203534-wpcom.
Summary: The file is synced with Jetpack, and D38340 cannot be merged into Jetpack as is as it introduces changes that will not pass our pre-commit hook. This diff fixes all PHPCS warnings. Test Plan: * Not much to test here, this only impacts documentation / comments. Reviewers: goldsounds Tags: #touches_jetpack_files Differential Revision: D39590 This commit syncs r r203555-wpcom.
jeherve
added
[Type] Bug
23 tasks
|
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: March 3, 2020. |
kraftbj
approved these changes
Mar 2, 2020
kraftbj
added
[Status] Ready to Merge
matticbot
added
[Status] Needs Changelog
and removed
[Status] Ready to Merge
jeherve
added a commit
that referenced
this pull request
Mar 31, 2020
* Initial changelog entry * Changelog: add #14904 * Changelog: add #14910 * Changelog: add #14913 * Changelog: add #14916 * Changelog: add #14922 * Changelog: add #14924 * Changelog: add #14925 * Changelog: add #14928 * Changelog: add #14840 * Changelog: add #14841 * Changelog: add #14842 * Changelog: add #14826 * Changelog: add #14835 * Changelog: add #14859 * Changelog: add #14884 * Changelog: add #14888 * Changelog: add #14817 * Changelog: add #14814 * Changelog: add #14819 * Changelog;: add #14797 * Changelog: add #14798 * Changelog: add #14802 * Changelog: add #13676 * Changelog: add #13744 * Changelog: add #13777 * Changelog: add #14446 * Changelog: add #14739 * Changelog: add #14770 * Changelog: add #14784 * Changelog: add #14897 * Changelog: add #14898 * Changelog: add #14968 * Changelog: add #14985 * Changelog: add #15044 * Changelog: add #15052 * Update to remove Podcast since it remains in Beta * Changelog: add #14803 * Changelog: add #15028 * Changelog: add #15065 * Changelog:add #14886 * Changelog: add #15118 * Changelog: add #14990 * Changelog: add #14528 * Changelog: add #15120 * Changelog: add #15126 * Changelog: add #15049 * Chanegelog: add #14852 * Changelog: add #15090 * Changelog: add #15138 * Changelog: add #15124 * Changelog:add #15055 * Changelog: add #15017 * Changelog: add #15109 * Changelog: add #15145 * Changelog:add #15096 * Changelog:add #15153 * Changelog: add #15133 * Changelog: add #14960 * Changelog: add #15127 * Changelog: add #15056 * Copy current changelog to changelog archive. * Clarify changelog description
westonruter
reviewed
Apr 7, 2020
| @@ -49,6 +52,28 @@ public static function init() { | |||
|
|
|||
| // Sync the amp-options. | |||
| add_filter( 'jetpack_options_whitelist', array( 'Jetpack_AMP_Support', 'filter_jetpack_options_whitelist' ) ); | |||
|
|
|||
| // Show admin bar. | |||
| add_filter( 'show_admin_bar', array( 'Jetpack_AMP_Support', 'show_admin_bar' ) ); | |||
There was a problem hiding this comment.
I do not think this is right. It is causing the admin bar to be hidden on all AMP pages, even when it should be displayed.
In AMP plugin 1.3 we introduced AMP dev mode support to exclude the admin bar from being subject to AMP validation constraints, allowing administrative scripts and styles to work as expected even on AMP pages.
More info: https://weston.ruter.net/2019/09/24/integrating-with-amp-dev-mode-in-wordpress/
There was a problem hiding this comment.
@jeherve if the issue is the scripts and styles that the Jetpack Likes module includes, a better way to resolve this I think is to explicitly mark them for AMP dev mode by adding admin-bar as a dependency for the scripts and styles.
There was a problem hiding this comment.
See also ampproject/amp-wp#4001
There was a problem hiding this comment.
Thanks for jumping in. We'll need to revisit this. I've logged your remarks in #15353.
This was referenced Apr 7, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR syncs:
Related: #9555
Changes proposed in this Pull Request:
This patch disables enqueuing of assets for some non-functional features
when in AMP mode.
Affected features:
Testing instructions:
Proposed changelog entry for your changes: