-
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 TargetVideo Alias To Brid AMP Player #40151
Conversation
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.
Otherwise LGTM
@@ -495,4 +496,5 @@ class AmpBridPlayer extends AMP.BaseElement { | |||
|
|||
AMP.extension(TAG, '0.1', (AMP) => { | |||
AMP.registerElement(TAG, AmpBridPlayer); | |||
AMP.registerElement(ALIAS, AmpBridPlayer); |
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.
It would be more proper to not use TAG
or ALIAS
here. TAG
is used for logging (e.g: Log.i(TAG, 'some error message') ) Here it is declaring the extension's tag name, so they are not TAGs. Just fill amp-brid-player
and amp-target-video-player
as strings would be good.
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.
Fixed!
@danijel-ristic Validator test failed. You need to update https://github.com/ampproject/amphtml/blob/main/extensions/amp-brid-player/validator-amp-brid-player.protoascii to have the validator recognize the new tag. Also I will suggest you to revert changes to examples/brid-player.amp.html for now. Once the validator is updated and published, you will be able to change the example then. |
@powerivq Done. |
Adding @banaag for validator rule approval. |
@danijel-ristic can you try doing a rebase? It should fix the failed test. |
Closes #40130
Talked to team and decided to go other way around then the issue suggests.