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

AMP link protection page #64

Merged
merged 5 commits into from
Nov 8, 2021
Merged

AMP link protection page #64

merged 5 commits into from
Nov 8, 2021

Conversation

SlayterDev
Copy link
Contributor

@SlayterDev SlayterDev commented Nov 2, 2021

This page will have sample links for AMP protection.

@SlayterDev SlayterDev requested a review from kdzwinel November 2, 2021 16:43
@SlayterDev SlayterDev marked this pull request as ready for review November 5, 2021 14:19
Copy link
Member

@kdzwinel kdzwinel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I left couple of comments.

Copy link
Member

@kdzwinel kdzwinel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Couple more questions about the links, feel free to merge yourself after that.

</li>
<li>
<p><a href="https://www.independent.co.uk/news/world/americas/us-politics/covid-vaccine-mandate-kids-san-francisco-b1951340.html?amp">?amp link</a></p>
<p class="expected">Expected: https://www.independent.co.uk/news/world/americas/us-politics/covid-vaccine-mandate-kids-san-francisco</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a good catch actually. I think independent in this case doesn't have the canonical element loaded when DOMContentLoaded fires. I have an idea to fix this.

</li>
<li>
<p><a href="https://www-wpxi-com.cdn.ampproject.org/v/s/www.wpxi.com/news/top-stories/pihl-bans-armstrong-student-section-hockey-games-after-vulgar-chants-directed-female-goalie/G2RP5FZA3ZDYRLFSWGDGQH2MY4/?amp_js_v=a6&amp_gsa=1&outputType=amp&usqp=mq331AQKKAFQArABIIACAw%3D%3D&fbclid=IwAR1TYTsjFEPy42VACpBR0n2jP-6whynOxwVqxpDMTWIRgW2vFlj573xVkEI#aoh=16358554203777&referrer=https%3A%2F%2Fwww.google.com&amp_tf=From%20%251%24s&ampshare=https%3A%2F%2Fwww.wpxi.com%2Fnews%2Ftop-stories%2Fpihl-bans-armstrong-student-section-hockey-games-after-vulgar-chants-directed-female-goalie%2FG2RP5FZA3ZDYRLFSWGDGQH2MY4%2F">ampproject.org with parameters</a></p>
<p class="expected">Expected: https://www.wpxi.com/news/top-stories/pihl-bans-armstrong-student-section-hockey-games-after-vulgar-chants-directed-female-goalie/G2RP5FZA3ZDYRLFSWGDGQH2MY4/?amp_js_v=a6&_gsa=1&outputType=amp&usqp=mq331AQKKAFQArABIIACAw%3D%3D&fbclid=IwAR1TYTsjFEPy42VACpBR0n2jP-6whynOxwVqxpDMTWIRgW2vFlj573xVkEI#aoh=16358554203777&referrer=https%3A%2F%2Fwww.google.com&_tf=From%20%251%24s&ampshare=https%3A%2F%2Fwww.wpxi.com%2Fnews%2Ftop-stories%2Fpihl-bans-armstrong-student-section-hockey-games-after-vulgar-chants-directed-female-goalie%2FG2RP5FZA3ZDYRLFSWGDGQH2MY4%2F</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is still an amp page (<html amp=true), although not served by google, is that expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of yes? This triggers the simple extraction. We're not recursively checking the links in case we get a double AMP link. I'll look in to the feasibility of this and update the page if needed.

@SlayterDev SlayterDev merged commit 1e12b2a into main Nov 8, 2021
@SlayterDev SlayterDev deleted the brad/amp branch November 8, 2021 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants