Using local storage for notification toggling#1644
Using local storage for notification toggling#1644dvoytenko merged 1 commit intoampproject:masterfrom
Conversation
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. |
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 |
|
Assigning to @cramforce for an additional review. |
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
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.