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

Save Network in a cookie #223

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

maxbethke
Copy link

Related to issue #220

Issue:
Refer to #220

What has been done:
Created a class for saving cookies to be able to have the choice the user made for his network beyond page reloads.

What has changed:
Store the choosen dropdown option in a cookie.
Restore the previously choosen option when the page is loaded.

@junderw
Copy link
Contributor

junderw commented Feb 1, 2021

  1. Why not localStorage?
  2. Are those ESNext private class fields? Those should be avoided, as browser support is still low.

But even if you were to use them, you declare them but never use them, and instead use the public name. You have to keep the # for every get and set of the field. This will throw a parsing error in any browser that doesn't support it. (unlike other unsupported functions where "if you don't touch the affected code you're fine"... this will break the entire file from being parsed so be careful.)

Example:

class Test {
 #name
 constructor(name) {
   this.name = name;
   this.#name = 'PRIVATE ' + name;
 }
 get privateName() {
   return this.#name
 }
}
var x = new Test('bob');
console.log(x.name)
// bob
console.log(x.privateName)
// PRIVATE bob

@maxbethke
Copy link
Author

  1. Haven't thought about that. What is the advantage of localStorage, apart from being easier to use?

  2. Woops, those should not be there anymore... I intented to use ESNext fields, but scratched the Idea due to low browser support.
    Also the whole class would only with with ES6 Support, which is not widely supported too yet, is it?

maxbethke and others added 3 commits February 1, 2021 14:50
Forgot  that they we're still there...
Using localStorage makes much more sense in this case.
not used anymore
@maxbethke
Copy link
Author

Did a bit of research on localStorage and came to the conclusion that in this case, it is the better way to do it.
Thanks for pointing that out to me! @junderw

Copy link
Contributor

@junderw junderw left a comment

Choose a reason for hiding this comment

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

This is only storing the network and causing the network to be "selected" on startup.
This ignores the possibility of a custom setup and a custom selection for API endpoints.

  1. Store all settings data.
  2. Restore it without triggering an event. Modify elements and variables. Maybe spread everything out into a function that can be used in the event handler and on boot when localStorage is present.


var o = ($("option:selected",this).attr("rel")).split(";");
optionSeleted = ($("option:selected",this).attr("rel")).split(";");
Copy link
Contributor

Choose a reason for hiding this comment

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

no var ?

Also, s/optionSeleted/optionSelected/

@@ -1932,33 +1934,35 @@ $(document).ready(function() {
});

$("#coinjs_coin").change(function(){
// set localStorage for use after page (re)load
localStorage.setItem(LOCALSTORAGE_COIN_KEY, $(this).val());
Copy link
Contributor

Choose a reason for hiding this comment

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

What about the custom attributes?

@ghost
Copy link

ghost commented May 16, 2022

@maxbethke Might worth to check out this PR as well #250, have added support for electrum servers as well.

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