Skip to content
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

Using local storage for notification toggling #1644

Merged
merged 1 commit into from
Jan 29, 2016

Conversation

dvoytenko
Copy link
Contributor

No description provided.

@@ -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 => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to do the assert earlier than this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is done early as well. Just being attentive to types.

@dvoytenko
Copy link
Contributor Author

@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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we either add a note here or on the PR to update validator. (or create an issue)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will, as soon as the PR is approved by @erwinmombay and @cramforce

@erwinmombay
Copy link
Member

@dvoytenko looks great, LGTM

@dvoytenko dvoytenko assigned cramforce and unassigned erwinmombay Jan 28, 2016
@dvoytenko dvoytenko changed the title POC: Using local storage for notification toggling Using local storage for notification toggling Jan 28, 2016
@dvoytenko
Copy link
Contributor Author

Assigning to @cramforce for an additional review.

'should have "data-dismiss-href" attribute.');
}

if (!this.win_.AMP_TEST) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not in tests? Comment please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed. Found another way to do this. But ultimately, I want unit-tests to be untainted by service integration - it's tested separately.

@dvoytenko
Copy link
Contributor Author

@cramforce changes are complete. PTAL.

@cramforce
Copy link
Member

LGTM

dvoytenko added a commit that referenced this pull request Jan 29, 2016
Using local storage for notification toggling
@dvoytenko dvoytenko merged commit f1bc9eb into ampproject:master Jan 29, 2016
@dvoytenko dvoytenko deleted the storage2 branch January 29, 2016 03:45

// Store and post.
if (this.storageKey_) {
this.storagePromise_.then(storage => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be done as a then callback inside the storageKey_ conditional in #shouldShow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, looking at the compressed diff view I thought this was in #shouldShowViaXhr_. Ignore my comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants