amphtml icon indicating copy to clipboard operation
amphtml copied to clipboard

✨ add validation for amp-social-share in amp-story

Open amandine-trl opened this issue 1 year ago • 8 comments

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.

amandine-trl avatar Jul 22 '24 10:07 amandine-trl

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Jul 22 '24 10:07 CLAassistant

@ychsieh could you take a look at this?

erwinmombay avatar Jul 24 '24 19:07 erwinmombay

Thanks for proposing the change! Could you include a screenshot and add the example html code in examples/amp-story?

ychsieh avatar Jul 25 '24 07:07 ychsieh

Capture d'écran 2024-07-25 170238 The screenshot corresponding to the added example

amandine-trl avatar Jul 25 '24 15:07 amandine-trl

I built amp-story-social-share.html locally but cannot see the button shown in your screenshot. Could you double check?

ychsieh avatar Jul 26 '24 05:07 ychsieh

Hi, I updated my example to add other cases than system share. Now, you should now be able to see at least the first three buttons. I think you may not be able to see the button because the sharing function is not activated in your browser. If you use chrome you maybe can check the web-share flag and set it to enabled if its not.

amandine-trl avatar Jul 26 '24 09:07 amandine-trl

Hi, I wanted to check in regarding the status of the merge request. Could you please let me know if it's ready to be merged and if there’s an estimated timeline for when that might happen?

Thank you for your help!

amandine-trl avatar Sep 23 '24 10:09 amandine-trl

You need owners approval. Adding @mszylkowski.

ychsieh avatar Sep 23 '24 14:09 ychsieh

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 main branch?

danielrozenberg avatar Jan 07 '25 19:01 danielrozenberg

Hi @danielrozenberg, The code has been updated and the issue is resolved. Let me know if there's anything else you'd like me to verify before merging.

amandine-trl avatar Jan 08 '25 10:01 amandine-trl

@amandine-trl we're all good, thanks for the contribution!

danielrozenberg avatar Jan 08 '25 19:01 danielrozenberg