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

Storing options in localstorage #136

Merged

Conversation

olleeriksson
Copy link
Contributor

@olleeriksson olleeriksson commented Nov 29, 2022

Hopefully this one is a little simpler. I've changed so that the options are now stored in local storage. I had it prepared since before.

I changed the remaining two options that used a boolean to store the option to a translation key instead so all options follow the same pattern, and it was also easier to store a text string in local storage.

Also, another quirk. I think the value of key_move_(enabled|disabled) and review_(slow|fast) had been reversed so I changed them. For example, the text of key_move_enabled was "Jump to key move: off" so I changed it to "on" instead so that the translation key correctly conveys what it means. In toggle_key_move() there was a similar mix-up which undid the confusion with the translation key. So everything worked before but the boolean value was just the reverse of what the option said.

And added a fallback in the switch-cases just to be safe if an option change name in the future.

BTW, I've got this option prepared as well. Do you want it? I can put up a PR as soon as this one is merged.

image

Copy link
Owner

@ArneVogel ArneVogel left a comment

Choose a reason for hiding this comment

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

One nit

*/
function set_options_values() {
let move_delay = document.getElementById("move_delay_time");
move_delay.innerHTML = move_delay_time;
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 innerHTML -> innerText? I know we control the input but I still would feel better with innerText :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely. There it is.

@ArneVogel ArneVogel merged commit db25c63 into ArneVogel:master Nov 30, 2022
@ArneVogel
Copy link
Owner

Thanks a lot. I like the get_option_from_localstorage solution 👍

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.

2 participants