Skip to content

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

Merged
processprocess merged 24 commits intoampproject:mainfrom
mszylkowski:ctaImageNone
Apr 18, 2022
Merged

🐛 [Story attachment] Fix cta-image=none showing link icon#38109
processprocess merged 24 commits intoampproject:mainfrom
mszylkowski:ctaImageNone

Conversation

@mszylkowski
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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