-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
block amp-analytics execution is consent if not resolved #1628
Conversation
7834fab
to
74a442c
Compare
* @private @const {?string} | ||
*/ | ||
this.consentNotificationId_ = this.element | ||
.getAttribute('data-consent-notification'); |
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.
data-consent-notification-id
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.
Looks good. Please add to respective |
74a442c
to
7e4d006
Compare
7e4d006
to
782d1fc
Compare
@cramforce: This change does not fix the same problem for amp-pixel. Shouldn't CLIENT-ID substitution in url-replacements handle this and throw an error if client id is not generated. Then amp-analytics and amp-pixel can react on that error and discard the hit. |
65cd125
to
e787779
Compare
821e5f4
to
b8744ec
Compare
@cramforce added test and document update. PTAL /cc @avimehta |
b8744ec
to
38e621c
Compare
</footer> | ||
|
||
|
||
<amp-analytics data-consent-notification-id="amp-user-notification1"> |
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 trim this example to only this?
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. removed everything (html, css) besides logo + notification + analytics.
@avimehta Sorry, I missed your question. This change (and a similar change for |
38e621c
to
20177e7
Compare
@cramforce PTAL |
<meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1"> | ||
<link href='https://fonts.googleapis.com/css?family=Georgia|Open+Sans|Roboto' rel='stylesheet' type='text/css'> | ||
<style amp-custom> | ||
body { |
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.
trim CSS
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.
LGTM |
9fe101d
to
2d26fc7
Compare
block amp-analytics execution is consent if not resolved
Closes #1510
Closes #1313