-
Notifications
You must be signed in to change notification settings - Fork 384
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
Eliminate amphtml
link relation from AMP-to-AMP linking
#6661
Conversation
Plugin builds for d49b860 are ready 🛎️!
|
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 this may be too conservative in its exclusion, by limiting it to only WebPage
, which is just one subtype of CreativeWork
. There are many others, like AboutPage
or AmpStory
. These also fail if there is a rel
attribute present. So I think the check should rather be for whether there is a property
attribute on the link, and if so, skip adding the rel
.
Or given that the amphtml
link relation has served no purpose since we introduced it, we should just eliminate adding it altogether.
Thanks @westonruter for your comment. What about the |
Adding it doesn't really have a purpose, no. As a page author you can disable AMP-to-AMP linking for an individual link if you give it the |
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.
Works as expected
Codecov Report
@@ Coverage Diff @@
## develop #6661 +/- ##
=============================================
+ Coverage 76.38% 77.58% +1.20%
- Complexity 6475 6476 +1
=============================================
Files 261 198 -63
Lines 20697 19519 -1178
=============================================
- Hits 15809 15144 -665
+ Misses 4888 4375 -513
Flags with carried forward coverage won't be shown. Click here to find out more. |
…amp-to-amp-attr * 'develop' of github.com:ampproject/amp-wp: (139 commits) Try fixing E2E tests Add missing comma Use current user option in Wizard template mode recommendation Increase JS unit test coverage, remove impossible cases E2E: test if template mode recommendation is marked as stale after scan results Convert template mode recommendation function into custom hook E2E: Ensure Reader Themes drawer is expanded before testing contents Disable Template Mode E2E tests on AMP Settings screen Fix E2E tests for Site Review panel on AMP Settings screen Fix E2E tests for Site Scan panel on AMP Settings screen Update Onboarding Wizard Site Scan E2E test Prevent running E2E tests on empty WordPress instance Refactor template mode recommendation logic; use it on AMP Settings Add test for when `gutenberg` isn't the only plugin for validation error Rename test to match tested file and remove impossible test case Update "Validated URLs" link text Ensure `gutenberg` is excluded only if there are other error sources Improve logic for getting plugin slug from file path Remove unused imports Ensure site scan is cancelled whenever AMP options are changed ...
amphtml
link relation from AMP-to-AMP linking
Summary
Fixes #6599
The
amphtml
link relation is no longer added since it has served no purpose. Additionally, thenoamphtml
relation is no longer added to the mobile version switcher link.Checklist