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 Sirdata to the list of supported CMPs in the amp-consent extension #26040

Merged
merged 9 commits into from
Jan 29, 2020
Merged

✨ Add Sirdata to the list of supported CMPs in the amp-consent extension #26040

merged 9 commits into from
Jan 29, 2020

Conversation

RemiSirdata
Copy link
Contributor

Following the I2I issue #24879, this PR adds Sirdata to the list of “Supported Consent Management Platforms” in the amp-consent extension.

Here’s an example of basic integration: https://cmp.sirdata.com/demo/amp.html
The example will be updated with the advanced integration once validated by AMP.

Corporate CLA signed since 11/21 for Sirdata linked to rde@sirdata.fr
Reference to @ampproject/wg-monetization

@tla-sirdata will maintain Sirdata AMP implementation.

Also fix a minor typo in css example file examples/amp-consent/cmp-vendors.amp.html

- update amp consent documentation & code to reference sirdata CMP
- fix css typo for style .filterBar form *
@googlebot
Copy link

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 with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@RemiSirdata
Copy link
Contributor Author

RemiSirdata commented Dec 16, 2019 via email

@zhouyx zhouyx requested review from zhouyx and removed request for jeffjose December 17, 2019 23:18
@RemiSirdata
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@RemiSirdata
Copy link
Contributor Author

@googlebot I fixed it.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@zhouyx
Copy link
Contributor

zhouyx commented Jan 14, 2020

Thank you for the Pull Request!
I'm testing with the example you provided, and I am seeing the following error. Please verify and let me know if it's an error on my end. THanks.
image

@zhouyx
Copy link
Contributor

zhouyx commented Jan 14, 2020

Another thing that I want to mention is we will no longer support the fullScreen API without user interaction. Please let us know if that works for your case. More details here. #26197

@tla-sirdata
Copy link

tla-sirdata commented Jan 15, 2020

I don't reproduce the error. Can you provide more information about the request ?
It may be because of an invalid clientConfig.

About the fullScreen API, we'll have to adjust our first screen to fit the maximum 60vh.

Copy link
Contributor

@zhouyx zhouyx left a comment

Choose a reason for hiding this comment

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

Looks great in general. Thank you for working around with our 60vh height restriction.

One thing to confirm with you is that: when I hit the close button on fullscreen, it leads me back to the following page instead of closing the prompt. Is that expected? Thank you
image

extensions/amp-consent/0.1/cmps.js Outdated Show resolved Hide resolved
extensions/amp-consent/cmps/sirdata.md Outdated Show resolved Hide resolved
examples/amp-consent/cmp-vendors.amp.html Show resolved Hide resolved
@tla-sirdata
Copy link

About the close button that leads back to the main screen instead of closing the prompt (only on the partners screen), it's indeed what it's expected by now. But since it might be confusing for users, we might change it.

@zhouyx
Copy link
Contributor

zhouyx commented Jan 16, 2020

Thanks for the clarification on the close button. Yes I found it to be a bit confusing, but it's your final call on the UI behavior within the iframe.

Could you please fix the http protocol and also sync to the latest master so that we can proceed and merge this Pull Request. Thank you

@lgtm-com
Copy link

lgtm-com bot commented Jan 20, 2020

This pull request introduces 1 alert when merging 7c7dbb7 into 6f79c36 - view on LGTM.com

new alerts:

  • 1 for Syntax error

@tla-sirdata
Copy link

Thanks for the clarification on the close button. Yes I found it to be a bit confusing, but it's your final call on the UI behavior within the iframe.

Could you please fix the http protocol and also sync to the latest master so that we can proceed and merge this Pull Request. Thank you

@zhouyx We removed the close button on the partners layout + all requested changes have been committed. Works for you ? Thanks

@zhouyx
Copy link
Contributor

zhouyx commented Jan 27, 2020

Looks great! Thank you

@zhouyx
Copy link
Contributor

zhouyx commented Jan 27, 2020

@tla-sirdata The travis check failed because of a dead link in the doc. I've created #26508 to fix it. Please sync to master again once that PR has been merged. Then I can proceed and merge this PR. Thank you very much.

@RemiSirdata
Copy link
Contributor Author

@tla-sirdata The travis check failed because of a dead link in the doc. I've created #26508 to fix it. Please sync to master again once that PR has been merged. Then I can proceed and merge this PR. Thank you very much.

Thx! I merged from master

@tla-sirdata
Copy link

@zhouyx It seems that there's still some failed checks. Let me know if we can help

@zhouyx zhouyx merged commit 4455a03 into ampproject:master Jan 29, 2020
@zhouyx
Copy link
Contributor

zhouyx commented Jan 29, 2020

restarted visual test, and everything looks good now. PR merged. Thank you for the integration.

@tla-sirdata
Copy link

Could you just tell me when it is in production ?
I will follow the discussion #26229 for TCFv2 support.

Thank you for your help!

@zhouyx
Copy link
Contributor

zhouyx commented Jan 30, 2020

The PR was merged on Wednesday, should be available for developer opt in sometime next Tuesday, and ship in prod the Tuesday afterwards. Here's our detailed schedule

Thank you

@tla-sirdata
Copy link

Got it ! Thank you

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.

6 participants