Skip to content

Found a bug in how value of toc-checkbox is handled #912

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 8, 2018
Merged

Found a bug in how value of toc-checkbox is handled #912

merged 2 commits into from
Nov 8, 2018

Conversation

@megoth
Copy link
Contributor

@megoth megoth commented Nov 7, 2018

Values on the checkbox might differ as it is now - this should handle it better

@ghost ghost assigned megoth Nov 7, 2018
@ghost ghost added the in progress label Nov 7, 2018
@megoth megoth requested a review from kjetilk November 7, 2018 16:35
kjetilk
kjetilk approved these changes Nov 7, 2018
View reviewed changes
Copy link
Member

@kjetilk kjetilk left a comment

Choose a reason for hiding this comment

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

OK... So, I can see that not-not is kinda the same as true, but why this important evades me...

I'm inclined to trust you on this, though.

@Ryuno-Ki
Copy link

Ryuno-Ki commented Nov 7, 2018

!!something casts something to a boolean.
something === 'true' compares something with the string (!) true.
true === 'true' yields false ;-)

See also https://github.com/denysdovhan/wtfjs#true-is-false

kjetilk
kjetilk requested changes Nov 7, 2018
View reviewed changes
Copy link
Member

@kjetilk kjetilk left a comment

Choose a reason for hiding this comment

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

Actually, there's a consistently failing test there now. So, it seems like the fix may have worked, but the test may not have been updated, or something?

@cory2067 cory2067 mentioned this pull request Nov 8, 2018
Closed
@cory2067
Copy link

cory2067 commented Nov 8, 2018

Can confirm this fixes issues I was having with registration on master

kjetilk
kjetilk approved these changes Nov 8, 2018
View reviewed changes
Copy link
Member

@kjetilk kjetilk left a comment

Choose a reason for hiding this comment

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

good, tests are passing!

@kjetilk kjetilk merged commit 059c4f7 into master Nov 8, 2018
@ghost ghost removed the in progress label Nov 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kjetilk kjetilk kjetilk approved these changes

Assignees

@megoth megoth

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@megoth @Ryuno-Ki @cory2067 @kjetilk