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

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

Merged

Conversation

amandine-trl
Copy link
Contributor

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.

@CLAassistant
Copy link

CLAassistant commented Jul 22, 2024

CLA assistant check
All committers have signed the CLA.

@erwinmombay erwinmombay requested a review from ychsieh July 24, 2024 19:45
@erwinmombay
Copy link
Member

@ychsieh could you take a look at this?

@ychsieh
Copy link
Contributor

ychsieh commented Jul 25, 2024

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

@amandine-trl
Copy link
Contributor Author

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

@ychsieh
Copy link
Contributor

ychsieh commented Jul 26, 2024

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

@amandine-trl
Copy link
Contributor Author

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.

Copy link
Contributor

@ychsieh ychsieh left a 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.

@amandine-trl
Copy link
Contributor Author

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!

@ychsieh ychsieh requested a review from mszylkowski September 23, 2024 14:15
@ychsieh
Copy link
Contributor

ychsieh commented Sep 23, 2024

You need owners approval. Adding @mszylkowski.

@danielrozenberg
Copy link
Member

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?

@amandine-trl
Copy link
Contributor Author

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.

@danielrozenberg danielrozenberg enabled auto-merge (squash) January 8, 2025 19:01
@danielrozenberg danielrozenberg merged commit b43318f into ampproject:main Jan 8, 2025
52 checks passed
@danielrozenberg
Copy link
Member

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

powerivq pushed a commit to powerivq/amphtml that referenced this pull request Jan 8, 2025
* 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
banaag added a commit to banaag/amphtml that referenced this pull request Jan 9, 2025
@banaag banaag mentioned this pull request Jan 9, 2025
banaag added a commit that referenced this pull request Jan 9, 2025
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants