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.

Sign up for GitHub

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

Jump to bottom

✅ [amp-google-read-aloud-player] Adds validator and examples for amp-google-read-aloud-player extension #37666

Merged
merged 3 commits into from
Feb 17, 2022
Merged

✅ [amp-google-read-aloud-player] Adds validator and examples for amp-google-read-aloud-player extension #37666

merged 3 commits into from
Feb 17, 2022

Conversation

mhalabi-google
Copy link
Contributor

@amp-owners-bot amp-owners-bot bot requested a review from dmanek February 13, 2022 12:22
@amp-owners-bot
Copy link

amp-owners-bot bot commented Feb 13, 2022 •
edited by ampprojectbot
Loading

Hey @ampproject/wg-caching! These files were changed:

extensions/amp-google-read-aloud-player/0.1/test/validator-amp-google-read-aloud-player.html
extensions/amp-google-read-aloud-player/0.1/test/validator-amp-google-read-aloud-player.out
extensions/amp-google-read-aloud-player/validator-amp-google-read-aloud-player.protoascii

Hey @alanorozco! These files were changed:

extensions/amp-google-read-aloud-player/amp-google-read-aloud-player.md

@alanorozco alanorozco changed the title ✅ [amp-google-read-aloud-player] Adds validator and examples for amp-google-read-aloud-player` extension ✅ [amp-google-read-aloud-player] Adds validator and examples for amp-google-read-aloud-player extension Feb 14, 2022
@alanorozco alanorozco requested review from MichaelRybak and removed request for dmanek February 14, 2022 17:30
MichaelRybak
MichaelRybak reviewed Feb 14, 2022
View reviewed changes
Copy link
Contributor

@MichaelRybak MichaelRybak left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good! Optionally add a few more tags into html that test the attributes below:

  • data-locale
  • data-ad-tag-url and it's value_url params
  • an http value for for data-url
  • a few more tests for the data-tracking-ids regexp:
    UA-1-123123
    ua-1-1
    123
    ua-1
    UA-1-1.2
    (empty string)
    UA-1-1 XYZ

extensions/amp-google-read-aloud-player/validator-amp-google-read-aloud-player.protoascii Outdated Show resolved Hide resolved
@MichaelRybak
Copy link
Contributor

@alanorozco Alan would you mind reviewing the example html, as well as giving a quick look to all the files too just in case? It's my first time reviewing a new extension.

@mhalabi-google
Copy link
Contributor Author

Looks good! Optionally add a few more tags into html that test the attributes below:

  • data-locale
  • data-ad-tag-url and it's value_url params
  • an http value for for data-url
  • a few more tests for the data-tracking-ids regexp:
    UA-1-123123
    ua-1-1
    123
    ua-1
    UA-1-1.2
    (empty string)
    UA-1-1 XYZ

Done. Thank you for the suggestions!

alanorozco
alanorozco approved these changes Feb 15, 2022
View reviewed changes
Copy link
Member

@alanorozco alanorozco left a comment

Choose a reason for hiding this comment

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

@MichaelRybak Example file LGTM.

mhalabi-google
mhalabi-google commented Feb 15, 2022
View reviewed changes
Copy link
Contributor Author

@mhalabi-google mhalabi-google 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 the quick review!

extensions/amp-google-read-aloud-player/validator-amp-google-read-aloud-player.protoascii Show resolved Hide resolved
@mhalabi-google
Copy link
Contributor Author

Thank you @MichaelRybak and @alanorozco for the review!

@alanorozco
Copy link
Member

@mhalabi-google Are you satisfied with the rules and test results? If so I'll merge

@mhalabi-google
Copy link
Contributor Author

@mhalabi-google Are you satisfied with the rules and test results? If so I'll merge

Yes, this can be merged. Thank you for the help!

@alanorozco alanorozco merged commit 7325ae2 into ampproject:main Feb 17, 2022
@luissegovia
Copy link

Alguien que me ayude a como implementar esto? estoy super perdido...

de donde genero el API KEY?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alanorozco alanorozco alanorozco approved these changes

@MichaelRybak MichaelRybak MichaelRybak approved these changes

Assignees
No one assigned
Labels
PR use: In LTS PR use: In Stable WG: caching
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mhalabi-google @MichaelRybak @alanorozco @luissegovia @amaltas @ampprojectbot