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

🐛 [Story attachment] Fix cta-image=none showing link icon #38109

Merged
merged 24 commits into from
Apr 18, 2022

Conversation

mszylkowski
Copy link
Contributor

@mszylkowski mszylkowski commented Apr 18, 2022

The cta-image=none on outlinks should hide the link icon, but due to a regression on #36706 the link icon was being shown. This PR hides the icon if cta-image=none for the open-page-attachment UI.

Added tests to verify this behavior so it's not broken in the future.

It now works (vis diff for no-image CTA was wrong before):
image

@mszylkowski mszylkowski self-assigned this Apr 18, 2022
@amp-owners-bot
Copy link

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.js
extensions/amp-story-page-attachment/0.1/test/test-amp-story-open-page-attachment.js

Copy link
Contributor

@processprocess processprocess left a comment

Choose a reason for hiding this comment

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

Thank you for catching, fixing and adding the unit test.

@gmajoulet
Copy link
Contributor

Can you link the bug report, with real world Story examples?

Is cta-image="none" a valid URL giving a valid AMP document that we've been supporting?

attrs: {
name: "cta-image"
value_url: {
protocol: "http"
protocol: "https"
}
}

@processprocess processprocess merged commit 089c845 into ampproject:main Apr 18, 2022
@mszylkowski
Copy link
Contributor Author

mszylkowski commented Apr 18, 2022

Can you link the bug report, with real world Story examples?

The bug was reported on Slack (so no issue I think), and could be tested with the local story attachment.html.

The validation seems to confirm the attribute set to "none" is valid, though I don't know exactly where this validation rule comes from. https://validator.ampproject.org/ seems to agree that it's valid AMP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants