-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
✨ add validation for amp-social-share in amp-story #40094
✨ add validation for amp-social-share in amp-story #40094
Conversation
@ychsieh could you take a look at this? |
Thanks for proposing the change! Could you include a screenshot and add the example html code in examples/amp-story? |
I built amp-story-social-share.html locally but cannot see the button shown in your screenshot. Could you double check? |
Hi, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the examples. This opens flexibility for implementation of different types of CTA for sharing, but a systematical approach would be a new extension("amp-story-social-share") that 1) provide a default CTA UX that is similar to page-attachment(reusing CSS should be possible) to ensure UX quality 2) encapsulate the functionalities of amp-social-share. Please add a todo at least.
Hi, Thank you for your help! |
You need owners approval. Adding @mszylkowski. |
Hi @amandine-trl, we'd like to merge this pull request, however the new test you've added is failing - would you please follow the fix instructions on the failed test, and rebase this on the latest |
…dine-trl/amphtml into amp-social-share-validation
Hi @danielrozenberg, |
@amandine-trl we're all good, thanks for the contribution! |
* add validation for amp-social-share in amp-story * Add amp-story with amp-social-share example * Update amp-story with amp-social-share example * Add TODO * add validation for amp-social-share in amp-story * Add amp-story with amp-social-share example * Update amp-story with amp-social-share example * Add TODO * Follow the fix instructions on the failed test
…rge conflicts, if any.
* cl/610820873 Fix validation for unexpected attribute name starting with = * cl/614818595 Fix validation for duplicate attribute names * cl/713373388 Two-way sync for PR #40094. No-op, or fixes merge conflicts, if any. * Update some files that failed validator test * Update some files that failed validator test
Closes #40014
Enable amp-social-share component within amp-story-page.
Allowing the addition of a the native social sharing component within the body of an amp-story-page.
For example, this would allow us to add a highly visible call-to-action, such as "Share this content" on the final page of the story.