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

Likely.initiate breaks counters of existing widgets #84

Merged
merged 1 commit into from
Aug 11, 2016
Merged

Conversation

vitkarpov
Copy link
Collaborator

@vitkarpov vitkarpov commented Aug 9, 2016

If options for update method are not specified, it will be empty object. {}.url never equals this.options.url, so the buttons try to reinit.

What happens next? Counters are being updated, and call the fetch method, which doesn't do anything if there's a corresponding counter and there's no force update. So the counters vanish.

@vitkarpov vitkarpov added the Bug label Aug 9, 2016
@vitkarpov
Copy link
Collaborator Author

@iamakulov

@iamakulov
Copy link
Collaborator

Cool, thanks! Will check this today.

@@ -62,7 +62,7 @@ Likely.prototype = {
update: function (options) {
if (
options.forceUpdate ||
options.url !== this.options.url
options.url && options.url !== this.options.url
Copy link
Collaborator

@iamakulov iamakulov Aug 10, 2016

Choose a reason for hiding this comment

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

Doesn’t it work the same? I.e.

options.url this.options.url expression result
undefined "ilyabirman.ru" false && false false
"ilyabirman.net" "ilyabirman.ru" true && false false
"ilyabirman.ru" "ilyabirman.ru" true && true true

The first two lines always result into false, so the additional options.url check doesn’t matter. Am I wrong?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

let's substitute the values:

// options.url === undefined

// false, `cause the first operand 
undefined && undefined !== 'some-default-url'

// true, `cause undefined literally doesn't equal the string
undefined !== 'some-default-url'

Copy link
Collaborator Author

@vitkarpov vitkarpov Aug 10, 2016

Choose a reason for hiding this comment

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

In another words: false || false && true => false (don't forget about priority of the operations, first && and then ||, so we don't need extra parenthesis)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, yes, !==, not ===. You’re right.

Copy link
Collaborator Author

@vitkarpov vitkarpov Aug 10, 2016

Choose a reason for hiding this comment

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

👍

@iamakulov
Copy link
Collaborator

Cool! Tested on journal.tinkoff.ru, seems to work. Thank you!

@iamakulov iamakulov merged commit 497562a into master Aug 11, 2016
@iamakulov iamakulov deleted the issue-83 branch August 11, 2016 10:39
@vitkarpov
Copy link
Collaborator Author

Happy to be involved in development of journal.tinkoff.ru 👍

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

Successfully merging this pull request may close these issues.

2 participants