-
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
[amp-experiment] Exposes isDismissed() method in AmpUserNotification #3832
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
/** | ||
* Copyright 2015 The AMP HTML Authors. All Rights Reserved. | ||
* Copyright 2016 The AMP HTML Authors. All Rights Reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
|
@@ -30,15 +30,6 @@ import {xhrFor} from '../../../src/xhr'; | |
const TAG = 'amp-user-notification'; | ||
|
||
|
||
/** | ||
* @export | ||
* @typedef {{ | ||
* elementId: string, | ||
* ampUserId: string | ||
* }} | ||
*/ | ||
let PostRequestMetadataDef; | ||
|
||
/** | ||
* @export | ||
* @typedef {{ | ||
|
@@ -239,21 +230,7 @@ export class AmpUserNotification extends AMP.BaseElement { | |
|
||
/** @override */ | ||
shouldShow() { | ||
let maybeCheckStoragePromise; | ||
|
||
if (this.persistDismissal_) { | ||
maybeCheckStoragePromise = this.storagePromise_.then(storage => { | ||
return storage.get(this.storageKey_); | ||
}); | ||
} else { | ||
// Skip reading storage when not data-persist-dismissal. | ||
maybeCheckStoragePromise = Promise.resolve(null); | ||
} | ||
|
||
return maybeCheckStoragePromise.catch(reason => { | ||
dev.error(TAG, 'Failed to read storage', reason); | ||
return false; | ||
}).then(dismissed => { | ||
return this.isDismissed().then(dismissed => { | ||
if (dismissed) { | ||
// Consent has been accepted. Nothing more to do. | ||
return false; | ||
|
@@ -286,6 +263,25 @@ export class AmpUserNotification extends AMP.BaseElement { | |
return this.dialogPromise_; | ||
} | ||
|
||
/** | ||
* Returns whether this notification has been dismissed and the dismissal | ||
* has been persisted in storage. Returns false if storage throws error or | ||
* 'data-persist-dismissal' is disabled. | ||
* @return {!Promise<boolean>} | ||
*/ | ||
isDismissed() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add the signature to |
||
if (!this.persistDismissal_) { | ||
return Promise.resolve(false); | ||
} | ||
return this.storagePromise_ | ||
.then(storage => storage.get(this.storageKey_)) | ||
.catch(reason => { | ||
dev.error(TAG, 'Failed to read storage', reason); | ||
return false; | ||
}) | ||
.then(persistedValue => !!persistedValue); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can move this into the previous promise block: .then(storage => storage.get(this.storageKey_))
.then(value => !!value, reason => {
dev.error(TAG); //...
return false;
}); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. Thanks |
||
} | ||
|
||
/** | ||
* Hides the current user notification and invokes the `dialogResolve_` | ||
* method. Removes the `.amp-active` class from the element. | ||
|
@@ -339,27 +335,26 @@ export class UserNotificationManager { | |
* Retrieve a promise associated to an `amp-user-notification` component | ||
* that is resolved when user agrees to the terms. | ||
* @param {string} id | ||
* @return {!Promise} | ||
* @return {!Promise<!AmpUserNotification>} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lets switch this one to |
||
*/ | ||
get(id) { | ||
this.managerReadyPromise_.then(() => { | ||
if (this.win_.document.getElementById(id) == null) { | ||
user.warn(TAG, `Did not find amp-user-notification element ${id}.`); | ||
} | ||
}); | ||
return this.getElementDeferById_(id).promise; | ||
return this.getOrCreateDeferById_(id).promise; | ||
} | ||
|
||
/** | ||
* Register an instance of `amp-user-notification`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wondering if we had a bug here. Should this be returned inside the managerReadyPromise chain? @erwinmombay There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looking at the code again, it shouldn't be a bug. although yes a bit weird. I don't think we need to be dependent on managerReady (viewer + dom ready) to return the promise (since that promise is resolved independently). this was really just to give the publisher dev a warning if they mistyped the notification "id". although you could argue as that being a user error instead of a warning and that we could move this dom scan to the url replacement code. |
||
* @param {string} id | ||
* @param {!UserNotification} userNotification | ||
* @param {!AmpUserNotification} userNotification | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this probably should have been |
||
* @return {!Promise} | ||
* @package | ||
*/ | ||
registerUserNotification(id, userNotification) { | ||
const deferred = this.getElementDeferById_(id); | ||
|
||
const deferred = this.getOrCreateDeferById_(id); | ||
// Compose the registered notifications into a promise queue | ||
// that blocks until one notification is dismissed. | ||
return this.nextInQueue_ = this.nextInQueue_ | ||
|
@@ -370,27 +365,19 @@ export class UserNotificationManager { | |
} | ||
}); | ||
}) | ||
.then(deferred.resolve) | ||
.then(deferred.resolve.bind(this, userNotification)) | ||
.catch(rethrowAsync.bind(null, | ||
'Notification service failed amp-user-notification', id)); | ||
} | ||
|
||
/** | ||
* Retrieve UserNotificationDeferDef object. | ||
* Retrieves UserNotificationDeferDef object. Creates an defer if it doesn't | ||
* exist. | ||
* @param {string} id | ||
* @return {!UserNotificationDeferDef} | ||
* @private | ||
*/ | ||
getElementDeferById_(id) { | ||
return this.createOrReturnDefer_(id); | ||
} | ||
|
||
/** | ||
* Create an defer if it doesnt exist, else just return the one in the | ||
* registry. | ||
* @return {!UserNotificationDeferDef} | ||
*/ | ||
createOrReturnDefer_(id) { | ||
getOrCreateDeferById_(id) { | ||
if (this.deferRegistry_[id]) { | ||
return this.deferRegistry_[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.
Our policy is that this is the creation year and it never changes.
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.
Reverted.