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

fix getBoolean on the Web #384

Merged
merged 2 commits into from Apr 19, 2022
Merged

fix getBoolean on the Web #384

merged 2 commits into from Apr 19, 2022

Conversation

eyousefifar
Copy link
Contributor

local storage saves values in string format. so Boolean('false') is always true. for this method to work correctly, it should check if value is 'true' or 'false' as string and returns the condition result.
to simplify the code I just return value === 'true'

web saves values in string format. so  'false' is always true
@mrousavy
Copy link
Owner

Hey, thanks for the PR this is a good change, I missed that one!

Comment on lines 45 to 52
/**
* local storage saves keys and values in utf-16 dom strings
* @link https://developer.mozilla.org/en-US/docs/Web/API/Window/localStorage#description
* true is saved as 'true', false as 'false'
* so if value == 'true' return true
* if value == 'false' return false
* I just simplified the conditions
*/
Copy link
Owner

Choose a reason for hiding this comment

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

Can you just remove this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrousavy done

remove comments
@mrousavy
Copy link
Owner

Great! Thanks for your PR 🙏

@mrousavy mrousavy merged commit 75b425d into mrousavy:master Apr 19, 2022
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.

None yet

2 participants