Skip to content

♻️ [amp-story-page-attachment] revise cta text in docs#36426

Merged
processprocess merged 5 commits intoampproject:mainfrom
processprocess:remove-cta-text
Nov 15, 2021
Merged

♻️ [amp-story-page-attachment] revise cta text in docs#36426
processprocess merged 5 commits intoampproject:mainfrom
processprocess:remove-cta-text

Conversation

@processprocess
Copy link
Copy Markdown
Contributor

This was not meant to be added to the docs for this component and can be deleted.
A comment is added about why the codepath is necessary in renderOutlinkUI
The demo text is updated to reflect the text in the images.

@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Oct 20, 2021

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

extensions/amp-story/1.0/amp-story-open-page-attachment.js
extensions/amp-story/amp-story-page-outlink.md

layout="nodisplay"
theme="dark">
<a href="https://www.google.com">Call To Action</a>
<a href="https://www.google.com"></a>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we keep the "Call to action" text here? I believe it can be used to configure the cta-text

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We decided to remove it since the default text is reflected in the image.
The following two examples include text corresponding to the example images.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We still need to document how people can tweak the call to action, should we update the screenshot instead of removing the configuration?

@processprocess processprocess changed the title 🚮 [amp-story-page-attachment] remove cta-text attribute from docs ♻️ [amp-story-page-attachment] revise cta text in docs Nov 15, 2021
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.

3 participants