Using local storage for notification toggling #1644
Conversation
| @@ -133,7 +150,7 @@ export class AmpUserNotification extends AMP.BaseElement { | |||
| * @private | |||
| */ | |||
| buildGetHref_(ampUserId) { | |||
| return this.urlReplacements_.expand(this.showIfHref_).then(href => { | |||
| return this.urlReplacements_.expand(assert(this.showIfHref_)).then(href => { | |||
There was a problem hiding this comment.
is it possible to do the assert earlier than this?
There was a problem hiding this comment.
It is done early as well. Just being attentive to types.
|
@erwinmombay added tests as well. ptal. |
| this.dismissHref_ = assertHttpsUrl( | ||
| this.element.getAttribute('data-dismiss-href'), this.element); | ||
| /** @private @const {?string} */ | ||
| this.showIfHref_ = this.element.getAttribute('data-show-if-href'); |
There was a problem hiding this comment.
can we either add a note here or on the PR to update validator. (or create an issue)
There was a problem hiding this comment.
I will, as soon as the PR is approved by @erwinmombay and @cramforce
|
@dvoytenko looks great, LGTM |
dvoytenko
changed the title
|
Assigning to @cramforce for an additional review. |
| 'should have "data-dismiss-href" attribute.'); | ||
| } | ||
|
|
||
| if (!this.win_.AMP_TEST) { |
There was a problem hiding this comment.
Why not in tests? Comment please
There was a problem hiding this comment.
Removed. Found another way to do this. But ultimately, I want unit-tests to be untainted by service integration - it's tested separately.
|
@cramforce changes are complete. PTAL. |
|
LGTM |
Using local storage for notification toggling
|
|
||
| // Store and post. | ||
| if (this.storageKey_) { | ||
| this.storagePromise_.then(storage => { |
There was a problem hiding this comment.
Shouldn't this be done as a then callback inside the storageKey_ conditional in #shouldShow?
There was a problem hiding this comment.
My bad, looking at the compressed diff view I thought this was in #shouldShowViaXhr_. Ignore my comments.
No description provided.