-
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
fix(amp-user-notification): generate ampUserId
by default
#1381
Conversation
2f964cc
to
bdd68ca
Compare
The id will be the same for this user going forward, but no other requests | ||
in AMP send the same id. | ||
You can use the id on your side to lookup/store whether the user has | ||
dismissed the notification before. | ||
Please not that the value of `ampUserId` may be the string `null` if no |
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.
"note"
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.
bdd68ca
to
c269098
Compare
cc @Meggin for docs review |
@cramforce per discussion, changing this to no argument |
The id will be the same for this user going forward, but no other requests | ||
in AMP send the same id. | ||
You can use the id on your side to lookup/store whether the user has | ||
dismissed the notification before. | ||
Please note that the value of `ampUserId` may be the string "null" if no | ||
fallback cookie value was found. | ||
- `showNotification` (boolean) - Boolean value wether the notification should be shown. |
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.
Spelling: 'wether' should be 'whether'. Also, I'd slightly re-word this: Boolean value indicating whether or not the notification should be shown.
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 once the two small comments have been actioned. |
c269098
to
da8c5ab
Compare
ampUserId
may be the string null
ampUserId
by default
c0eec61
to
81daf9a
Compare
return cid.get('amp-user-notification', | ||
Promise.resolve(), this.dialogPromise_); | ||
return cid.get( | ||
{scope: 'amp-user-notification', createCookieIfNotPresent: 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.
@cramforce PTAL. (this is for amp-user-notification cid request)
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.
/cc @dvoytenko for additional review
LGTM |
Looks good to me. Pending others' comments. |
81daf9a
to
ed1bce3
Compare
fix(amp-user-notification): generate `ampUserId` by default
No description provided.