Dynamic playlist tag support (amp-brid-player)

[grajzer]

Added support for tags in dynamic playlists, and updated documentation for new attribute (#19190)

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[grajzer]

CLA signed.

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[honeybadgerdontcare]

I see in the example that data-playlist has text which may be why this restriction is being removed. However removing this restriction allows the value to be anything (including nothing). That doesn't seem ideal. Would (\\d|\\w)+ be sufficient for a valid value? Are there other valid characters in a non-id playlist name?

May also need to update the documentation for data-playlist stating what is allowed. Currently it states ID which till now was just a number.

[grajzer]

Would it be OK if we put .+ because we would like to enable users to put anything there? Will update the documentation as you suggested. Thanks.

[honeybadgerdontcare]

That is fine. I just want to be sure it's what one would want as it's to prevent someone from doing something wrong unintentionally.

[grajzer]

The requested changes are implemented now.

[alanorozco]

HI @grajzer. Thanks for contributing.

I see this only makes validator changes. I assume this is already handled in the current src resolution logic?

amphtml/extensions/amp-brid-player/0.1/amp-brid-player.js

Lines 105 to 126 in 0389aa3

	const itemsNum = this.element.hasAttribute('data-dynamic') ? '10' : '1';
	if (this.element.hasAttribute('data-video')) {
	feedType = 'video';
	} else if (this.element.hasAttribute('data-dynamic')) {
	feedType = this.element.getAttribute('data-dynamic');
	} else if (this.element.hasAttribute('data-playlist')) {
	feedType = 'playlist';
	} else if (this.element.hasAttribute('data-outstream')) {
	feedType = 'outstream';
	}
	//Create iframe
	const src = 'https://services.brid.tv/services/iframe/' +
	encodeURIComponent(feedType) +
	'/' + encodeURIComponent(this.feedID_) +
	'/' + encodeURIComponent(this.partnerID_) +
	'/' + encodeURIComponent(this.playerID_) + '/0/' + itemsNum + '/?amp=1';
	this.videoIframeSrc_ = assertAbsoluteHttpOrHttpsUrl(src);
	return this.videoIframeSrc_;

If so, we should probably rename itemsNum to something more appropriate.

[grajzer]

Hi @alanorozco, you're right... everything was already handled there and we only need to fix the validation.

The itemsNum really tells how many items to request from our backend and can be 1 or 10, so it would be great if we can keep the name:

const itemsNum = this.element.hasAttribute('data-dynamic') ? '10' : '1';

Thanks for taking a look.

[alanorozco]

Right! I confused myself with the param order. Nothing on my end from the runtime side, just waiting for validation approval so I can merge. 👍

[honeybadgerdontcare]

validation looks good

[grajzer]

Hi guys, need to ask if I have to do anything else, or you're going to take the merging/deploying process from here? If latter, how long it usually take for code to go live? This is my first change, so please don't mind the question.

[alanorozco]

@grajzer Merged! Please track our releases for this feature.