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

✨ Brid player support for child JSON config #38984

Merged
merged 3 commits into from
Aug 4, 2023

Conversation

grajzer
Copy link
Contributor

@grajzer grajzer commented May 19, 2023

Support for providing additional config using child JSON.

@amp-owners-bot amp-owners-bot bot requested a review from nainar May 19, 2023 10:31
@amp-owners-bot
Copy link

Hey @alanorozco! These files were changed:

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

@erwinmombay erwinmombay requested review from erwinmombay, powerivq and ychsieh and removed request for nainar May 31, 2023 19:26
@erwinmombay
Copy link
Member

@grajzer are you affiliated with brid?

const configElement = doc.createElement('script');
configElement.setAttribute('type', 'application/json');
if (typeof config == 'string') {
configElement.textContent = config;
Copy link
Member

Choose a reason for hiding this comment

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

eozmen410 do we need to make this assignment trusted types compatible?

@grajzer
Copy link
Contributor Author

grajzer commented Jun 1, 2023

@erwinmombay Yes, I work in Brid and I made all previous changes to this component.

@grajzer
Copy link
Contributor Author

grajzer commented Jun 15, 2023

Hi @erwinmombay please let me know if anything else is needed in regards to this, or when can we expect this change to go live.

@erwinmombay
Copy link
Member

@grajzer apologies for late response.could you rebase and push the code out again. trying to get this green to merge

@CLAassistant
Copy link

CLAassistant commented Jul 28, 2023

CLA assistant check
All committers have signed the CLA.

@grajzer
Copy link
Contributor Author

grajzer commented Jul 28, 2023

Hi @erwinmombay, everything is updated now.

@powerivq powerivq merged commit 3ecc395 into ampproject:main Aug 4, 2023
10 checks passed
eszponder pushed a commit to krzysztofequativ/amphtml that referenced this pull request Apr 22, 2024
* Brid player support for child JSON config

* Brid player fix lint errors
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.

5 participants