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
Introduce consentHandlingOverride
for amp-ad
#15285
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add blockOnConsent
here in this PR ?
@@ -160,6 +161,7 @@ export const adConfig = { | |||
remoteHTMLDisabled: true, | |||
masterFrameAccessibleType: 'google_network', | |||
fullWidthHeightRatio: 1.2, | |||
consentHandlingOverride: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to document this here. https://github.com/ampproject/amphtml/blob/master/ads/_config.js#L56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need documentation here : )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
3p/3p.js
Outdated
@@ -272,6 +272,7 @@ function validateAllowedFields(data, allowedFields) { | |||
location: true, | |||
mode: true, | |||
consentNotificationId: true, | |||
consentHandlingOverride: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be blockOnConsent
, the attribute is data-block-on-consent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't see the name change : )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woops. really done now. thanks!
@@ -160,6 +161,7 @@ export const adConfig = { | |||
remoteHTMLDisabled: true, | |||
masterFrameAccessibleType: 'google_network', | |||
fullWidthHeightRatio: 1.2, | |||
consentHandlingOverride: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need documentation here : )
* Introduce `consentHandlingOverride` for amp-ad * Introduce `consentHandlingOverride` for amp-ad * address comments * address comments * Fix presubmit * Extra lint fixes. * woops * Fix type-checks. * Fix type-checks.
Closes #15269