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 TargetVideo Alias To Brid AMP Player #40151

Merged
merged 3 commits into from
Sep 19, 2024

Conversation

danijel-ristic
Copy link
Contributor

@danijel-ristic danijel-ristic commented Sep 17, 2024

Closes #40130

Talked to team and decided to go other way around then the issue suggests.

Copy link
Contributor

@powerivq powerivq left a 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);
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@powerivq
Copy link
Contributor

powerivq commented Sep 18, 2024

@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.

@danijel-ristic
Copy link
Contributor Author

@powerivq Done.

@powerivq powerivq requested a review from banaag September 18, 2024 12:24
@powerivq
Copy link
Contributor

Adding @banaag for validator rule approval.

@powerivq
Copy link
Contributor

@danijel-ristic can you try doing a rebase? It should fix the failed test.

@powerivq powerivq merged commit 51f6e4c into ampproject:main Sep 19, 2024
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename Brid player to TargetVideo
5 participants