Allow opting-out of AMP-to-AMP links, ensure 'Exit Reader Mode' link is non-AMP #4146
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
As Weston suggested, this forces links to non-AMP.
Even if add_filter( 'amp_to_amp_linking_enabled', '__return_true' ); is present, this should still go to non-AMP.
When in Reader or Transitional mode, links to the same origin will usually be to AMP. But this filter enables passing URLs that will link to non-AMP, not AMP.
The if block only ran if the previous block didn't, so an elseif block probably makes sense here.
It's not very clear what these are testing without the comments.
There's no need to store this in a variable, So simply move the boolean to the conditional.
This is clearer that it's in the AMP plugin, as the beginning serves as a sort of prefix.
kienstra
commented
Jan 21, 2020
westonruter
reviewed
Jan 21, 2020
The rel attribute can have multiple space-separated values: https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types
Commit Weston's suggestion Co-Authored-By: Weston Ruter <[email protected]>
Pass a 3rd argument of true to array_search() so that Travis passes.
Now that it's changed to noamphtml, update this in the unit tests also.
kienstra
changed the title
westonruter
requested changes
Jan 23, 2020
The rel value was changed to noamphtml, so it should be reflected here also. Co-Authored-By: Weston Ruter <[email protected]>
Using Weston's suggestion in the pull request.
As Weston pointed out, this is filtering URLs.
These are not really links, they're URLs.
As Weston mentioned, this shouldn't be used in comparison.
Co-Authored-By: Weston Ruter <[email protected]>
As Weston mentioned, this didn't have one.
Do npm run lint:php:fix to fix some of these.
westonruter
approved these changes
Jan 23, 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.
Summary
This allows opting out of AMP-to-AMP links via the filter
'excluded_links_from_amp_to_amp'or addingrel="noamphtml"to the<a>.Rel attribute
...will produce a link to a non-AMP URL, with the
relstripped:Filter
'Exit Reader Mode' Link
Even with
add_filter( 'amp_to_amp_linking_enabled', '__return_true' );, the 'Exit Reader Mode' link will now go to non-AMP:Fixes #3689
Checklist