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

Include yoast-seo-adminbar style in AMP dev mode #13502

Merged
merged 2 commits into from Sep 20, 2019

Conversation

@westonruter
Copy link
Contributor

commented Sep 19, 2019

Summary

In ampproject/amp-wp#3187 the AMP plugin added support for the new AMP dev mode. This allows normally invalid elements to be exempted from validation when being used in certain contexts, such as when the admin bar is being shown to authenticated users. AMP plugin v1.3 prevents the admin bar CSS from being processed and thus it is not counted against the 50KB limit in the style[amp-custom] stylesheet. This PR does the same for the admin bar CSS added by WordPress SEO, which avoids the CSS from being tree-shaken/converted and it prevents it from being counted against the 50KB limit.

Before

The CSS is included in the custom styles:

<!--
The style[amp-custom] element is populated with:
 22455 B (46%): link#twentynineteen-style-css[rel=stylesheet][href=https://wordpressdev.lndo.site/core-dev/src/wp-content/themes/twentynineteen/style.css?ver=1.4][media=all]
     0 B: style#twentynineteen-style-inline-css
  1033 B (75%): link#twentynineteen-print-style-css[rel=stylesheet][href=https://wordpressdev.lndo.site/core-dev/src/wp-content/themes/twentynineteen/print.css?ver=1.4][media=print]
  2014 B (79%): link#yoast-seo-adminbar-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/wordpress-seo/css/dist/adminbar-1210.min.css?ver=12.1][media=all]
   122 B: style
    39 B: style[media=print]
Total included size: 25,663 bytes (48% of 52,539 total after tree shaking)
-->

After

<!--
The style[amp-custom] element is populated with:
 22455 B (46%): link#twentynineteen-style-css[rel=stylesheet][href=https://wordpressdev.lndo.site/core-dev/src/wp-content/themes/twentynineteen/style.css?ver=1.4][media=all]
     0 B: style#twentynineteen-style-inline-css
  1033 B (75%): link#twentynineteen-print-style-css[rel=stylesheet][href=https://wordpressdev.lndo.site/core-dev/src/wp-content/themes/twentynineteen/print.css?ver=1.4][media=print]
   122 B: style
    39 B: style[media=print]
Total included size: 23,649 bytes (47% of 50,008 total after tree shaking)
-->
...
<link rel="stylesheet" id="yoast-seo-adminbar-css" href="https://wordpressdev.lndo.site/content/plugins/wordpress-seo/css/dist/adminbar-1210.min.css?ver=12.1" media="all" data-ampdevmode="">

This PR can be summarized in the following changelog entry:

  • Include admin bar CSS in AMP dev mode.

Relevant technical choices:

  • The yoast-seo-adminbar style is simply marked as being dependent on the core admin-bar stylesheet; the AMP plugin uses this as a signal to automatically add the data-ampdevmode attribute to the printed stylesheet link tag. The amp_dev_mode_element_xpaths filter is being used to identify the link elements after the page has rendered, but this could have alternatively used the style_loader_tag filter to inject the data-ampdevmode attribute instead during rendering.

Test instructions

This PR can be tested by following these steps:

  1. Activate AMP v1.3-RC1 and enable Transitional or Standard mode.
  2. With the admin bar being shown on the frontend, navigate to an AMP page.
  3. Look at the page source and verify that WordPress SEO admin bar CSS is not concatenated into style[amp-custom].

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Documentation

  • I have written documentation for this change.

Quality assurance

  • I have tested this code to the best of my abilities
  • I have added unittests to verify the code works as intended
@westonruter

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

Actually, if the stylesheet simply adds admin-bar as a dependency, that may be all that is needed here. If the dependency is set then the AMP plugin will automatically include it in dev mode.

@westonruter

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

Done in 8456d66. That simplifies this a lot, and it doesn't add anything AMP-specific to the plugin.

@Djennez

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2019

Hi @westonruter and thank you for the PR! One of our developers will have a look at it.

@jdevalk

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

Thanks Weston, that's simple enough. Merging.

@jdevalk jdevalk merged commit ff0f2a8 into Yoast:trunk Sep 20, 2019
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@IreneStr IreneStr added this to the 12.3 milestone Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.