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

Eliminate amphtml link relation from AMP-to-AMP linking #6661

Merged
merged 14 commits into from
Oct 29, 2021

Conversation

bartoszgadomski
Copy link
Contributor

@bartoszgadomski bartoszgadomski commented Oct 26, 2021

Summary

Fixes #6599

The amphtml link relation is no longer added since it has served no purpose. Additionally, the noamphtml relation is no longer added to the mobile version switcher link.

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@CLAassistant
Copy link

CLAassistant commented Oct 26, 2021

CLA assistant check
All committers have signed the CLA.

@bartoszgadomski bartoszgadomski marked this pull request as ready for review October 26, 2021 20:19
@github-actions
Copy link
Contributor

github-actions bot commented Oct 26, 2021

Plugin builds for d49b860 are ready 🛎️!

@westonruter westonruter requested review from milindmore22 and removed request for schlessera and delawski October 26, 2021 20:33
Copy link
Member

@westonruter westonruter left a 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.

@westonruter westonruter added this to the v2.2 milestone Oct 26, 2021
@bartoszgadomski
Copy link
Contributor Author

Thanks @westonruter for your comment. What about the rel=noamphtml, does this have any purpose or can be removed as well?

@westonruter
Copy link
Member

What about the rel=noamphtml, does this have any purpose or can be removed as well?

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 noamphtml relation. But otherwise, there's not any known need to add the relation when AMP-to-AMP is being excluded

Copy link
Collaborator

@milindmore22 milindmore22 left a 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
Copy link

codecov bot commented Oct 29, 2021

Codecov Report

Merging #6661 (f1627b6) into develop (0478c24) will increase coverage by 1.20%.
The diff coverage is 100.00%.

❗ Current head f1627b6 differs from pull request most recent head d49b860. Consider uploading reports for the commit d49b860 to get more accurate results
Impacted file tree graph

@@              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     
Flag Coverage Δ
javascript ?
php 77.58% <100.00%> (+0.06%) ⬆️
unit 77.58% <100.00%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
includes/sanitizers/class-amp-link-sanitizer.php 85.71% <100.00%> (+13.54%) ⬆️
src/MobileRedirection.php 83.51% <100.00%> (ø)
...src/components/site-scan-context-provider/index.js
...block-validation/components/error/error-content.js
assets/src/components/clipboard-button/index.js
assets/src/components/svg/check.js
assets/src/components/svg/standard.js
assets/src/block-editor/store/selectors.js
assets/src/components/amp-drawer/index.js
...mponents/use-template-mode-recommendation/index.js
... and 56 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
  ...
@westonruter westonruter enabled auto-merge October 29, 2021 17:21
@westonruter westonruter merged commit fb27fb8 into develop Oct 29, 2021
@westonruter westonruter deleted the fix/6599-amp-to-amp-attr branch October 29, 2021 17:28
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 15, 2021
@westonruter westonruter changed the title Eliminate AMP to AMP linking attribute from links Eliminate amphtml link relation from AMP-to-AMP linking Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Structured data broken by amphtml link relation in AMP-to-AMP linking
4 participants