✨ ✅ 🐛 Add new data attributes to `amp-jwplayer` #35163

mamaddox · 2021-07-09T13:44:38Z

This PR will...

Add support for additional data attributes for the amp-jwplayer extension.
Gather consent data.
Fix duplicate analytic events being emitted during ad playback by removing incorrect event mappings.

Relates to:

Documentation PR: 📖 Update amp-jwplayer documentation #35181

CLAassistant · 2021-07-09T13:44:44Z

All committers have signed the CLA.

kristoferbaxter · 2021-07-20T22:26:03Z

Adding @alanorozco to the issue, and I should be able to look tomorrow.

kristoferbaxter

Left a few comments and a nit, adding @alanorozco to review as well.

extensions/amp-jwplayer/0.1/amp-jwplayer.js

kristoferbaxter · 2021-07-21T17:31:49Z

extensions/amp-jwplayer/0.1/amp-jwplayer.js

+    const configJSON = element.getAttribute('data-config-json');
+    const config = tryParseJson(configJSON) || {};
+
+    Object.keys(configAttributes).forEach((attr) => {


@alanorozco Thoughts? This appears to be a filter + copy.

It looks like data-config-json and data-config-json-* are equivalent with the latter overriding specific properties in the former. It's the same case with data-ad-cust-params and adMacro*

I would suggest that we stick to a single API, either a single attribute with all properties, or single attributes setting every property. But not both.

There are actually only three options for the data-config-* attribute, data-config-skin, data-config-plugin, and data-config-json, so data-config-json-* attributes are not expected. They follow that data-config-* naming convention because they are player config settings.

getDataParamsFromAttributes was used to grab those first two together, but they can instead be explicitly grabbed like this:

const configSkin = element.getAttribute('data-config-skin'); const configPlugin = element.getAttribute('data-config-plugin');

The data-ad-* attributes are similar in that they are player ad settings. So data-ad-cust-params is standalone, but multiple adMacros can be set with data-ad-macro-*.

All these attributes are set on the config object to be sent together to the player, but that object can be renamed or a separate one can be used to make it more clear.
Open to any thoughts and ideas.

kristoferbaxter · 2021-07-21T17:40:29Z

I've kicked off the systems to run CI/CD as well.

kristoferbaxter · 2021-07-23T16:09:48Z

@mamaddox Can you follow the instructions here to get this build running on CI?

Pasted for convenience.

ERROR: pull/35163 is missing the latest CircleCI config revision e1c2d0fbaf2e37c36e57474f74129601714c3795.


This can be fixed in three ways:


1. Click the "Update branch" button on GitHub and follow instructions.
   ⤷ It can be found towards the bottom of the PR, after the list of checks.


2. Pull the latest commits from main and re-push the PR branch.
   ⤷ Follow these steps:

      git checkout main
      git pull
      git checkout <PR branch>
      git merge main
      git push origin


3. Rebase on main and re-push the PR branch.
   ⤷ Follow these steps:

      git checkout main
      git pull
      git checkout <PR branch>
      git rebase main
      git push origin --force

This reverts commit 5aeba5a.

* Add setup method * create separate postMessage method and add setup test * adding support for data-params and querystring * updating tests for data-params * re-adding unit test i accidentally removed * Pass custom config added through amp to player * Use correct import format * updated attribute names * adding code suggestions * Add test coverage and fix formatting * Move test config to above assertation * Update media id and player id for vast ad config * Add cust-params data attribute for ads * testing * Integrate with amp-consent and add custom macros * Lint fixes Co-authored-by: Daniel Lee <[email protected]>

mamaddox · 2021-07-28T19:06:11Z

@kristoferbaxter Rebased the branch and fixed the other CircleCI issues, so should be good now.

kristoferbaxter · 2021-07-30T19:18:02Z

Approved the CI pipeline.

Update our amp-jwplayer docs with the new attributes Add amp-jwplayer in the docs where it applies

…params

aliciahurst · 2021-09-07T15:11:41Z

hi @kristoferbaxter! just a quick nudge from the JW Player side, any update on when this will be merged and released?

This reverts commit e73aba1.

* Add JW Player to documentation that applies * Revert "Add JW Player to documentation that applies" This reverts commit 5aeba5a. * Extend AMP Params (ampproject#62) * Add setup method * create separate postMessage method and add setup test * adding support for data-params and querystring * updating tests for data-params * re-adding unit test i accidentally removed * Pass custom config added through amp to player * Use correct import format * updated attribute names * adding code suggestions * Add test coverage and fix formatting * Move test config to above assertation * Update media id and player id for vast ad config * Add cust-params data attribute for ads * testing * Integrate with amp-consent and add custom macros * Lint fixes Co-authored-by: Daniel Lee <[email protected]> * Remove bad import * Clean up consentgdpr * Add comment about appending querystring * Clean up from rebase * Remove trailing whitespaces * Remove controls attribute * 📖 [JW8-11873] Update JW Player AMP Docs (ampproject#61) Update our amp-jwplayer docs with the new attributes Add amp-jwplayer in the docs where it applies * cleanup rebase * lint fixes Co-authored-by: Zack Haigh <[email protected]> Co-authored-by: Daniel Lee <[email protected]>

mamaddox changed the title ~~✨ ✅ 🐛 Add new data attributes to amp-jwplayer~~ ✨ ✅ 🐛 📖 Add new data attributes to amp-jwplayer Jul 12, 2021

mamaddox changed the title ~~✨ ✅ 🐛 📖 Add new data attributes to amp-jwplayer~~ ✨ ✅ 🐛 Add new data attributes to amp-jwplayer Jul 12, 2021

mamaddox mentioned this pull request Jul 12, 2021

📖 Update amp-jwplayer documentation #35181

Merged

mamaddox marked this pull request as ready for review July 12, 2021 15:21

amp-owners-bot bot requested a review from dmanek July 12, 2021 15:21

kristoferbaxter requested a review from alanorozco July 20, 2021 22:25

kristoferbaxter reviewed Jul 21, 2021

View reviewed changes

kristoferbaxter removed the request for review from dmanek July 21, 2021 17:33

mamaddox requested a review from kristoferbaxter July 22, 2021 20:23

kristoferbaxter approved these changes Jul 23, 2021

View reviewed changes

alanorozco approved these changes Jul 23, 2021

View reviewed changes

mamaddox force-pushed the extend-amp-params branch from 4e9abec to 765b171 Compare July 26, 2021 16:32

Zack Haigh and others added 7 commits July 27, 2021 16:33

Add JW Player to documentation that applies

caa3227

Revert "Add JW Player to documentation that applies"

49b2a4f

This reverts commit 5aeba5a.

Remove bad import

df4d9eb

Clean up consentgdpr

076ded0

Add comment about appending querystring

89da1ff

Clean up from rebase

77dc0b4

mamaddox force-pushed the extend-amp-params branch from ce703e7 to 77dc0b4 Compare July 27, 2021 21:37

mamaddox added 2 commits July 27, 2021 16:50

Remove trailing whitespaces

d23a21d

Remove controls attribute

566f898

mamaddox and others added 2 commits August 2, 2021 10:31

📖 [JW8-11873] Update JW Player AMP Docs (#61)

bb027d2

Update our amp-jwplayer docs with the new attributes Add amp-jwplayer in the docs where it applies

Merge branch 'main' into extend-amp-params

09f7fb8

zetagame added 4 commits August 23, 2021 11:58

cleanup rebase

8a00988

lint fixes

7f7f398

Merge branch 'main' of github.com:ampproject/amphtml into extend-amp-…

d0c304f

…params

Merge branch 'main' of github.com:ampproject/amphtml into extend-amp-…

332f6dc

…params

kristoferbaxter merged commit e73aba1 into ampproject:main Sep 9, 2021

kristoferbaxter added a commit that referenced this pull request Sep 9, 2021

Revert "✨ ✅ 🐛 Add new data attributes to amp-jwplayer (#35163)"

f538b64

This reverts commit e73aba1.

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels Sep 16, 2021

ampprojectbot added the PR use: In LTS label Oct 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ ✅ 🐛 Add new data attributes to `amp-jwplayer` #35163

✨ ✅ 🐛 Add new data attributes to `amp-jwplayer` #35163

mamaddox commented Jul 9, 2021 •

edited

Loading

CLAassistant commented Jul 9, 2021 •

edited

Loading

kristoferbaxter commented Jul 20, 2021

kristoferbaxter left a comment

kristoferbaxter Jul 21, 2021

alanorozco Jul 21, 2021

mamaddox Jul 22, 2021

kristoferbaxter commented Jul 21, 2021

kristoferbaxter commented Jul 23, 2021

mamaddox commented Jul 28, 2021

kristoferbaxter commented Jul 30, 2021

aliciahurst commented Sep 7, 2021

✨ ✅ 🐛 Add new data attributes to amp-jwplayer #35163

✨ ✅ 🐛 Add new data attributes to amp-jwplayer #35163

Conversation

mamaddox commented Jul 9, 2021 • edited Loading

This PR will...

Relates to:

CLAassistant commented Jul 9, 2021 • edited Loading

kristoferbaxter commented Jul 20, 2021

kristoferbaxter left a comment

Choose a reason for hiding this comment

kristoferbaxter Jul 21, 2021

Choose a reason for hiding this comment

alanorozco Jul 21, 2021

Choose a reason for hiding this comment

mamaddox Jul 22, 2021

Choose a reason for hiding this comment

kristoferbaxter commented Jul 21, 2021

kristoferbaxter commented Jul 23, 2021

mamaddox commented Jul 28, 2021

kristoferbaxter commented Jul 30, 2021

aliciahurst commented Sep 7, 2021

✨ ✅ 🐛 Add new data attributes to `amp-jwplayer` #35163

✨ ✅ 🐛 Add new data attributes to `amp-jwplayer` #35163

mamaddox commented Jul 9, 2021 •

edited

Loading

CLAassistant commented Jul 9, 2021 •

edited

Loading