-
Notifications
You must be signed in to change notification settings - Fork 60
Allow to save and manage multiple accounts in local storage - Closes #573 #966
Conversation
54abcd5
to
ec17131
Compare
ec17131
to
097fcd8
Compare
24127d0
to
04921d1
Compare
1d6d6e2
to
aab4ee4
Compare
a1d1073
to
3461c07
Compare
src/utils/savedAccounts.js
Outdated
@@ -0,0 +1,41 @@ | |||
export const getSavedAccounts = () => { | |||
const savedAccounts = localStorage.getItem('accounts'); | |||
let accounts = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you initiate an array if your going to re-assign it?
src/utils/savedAccounts.js
Outdated
const savedAccounts = localStorage.getItem('accounts'); | ||
let accounts = []; | ||
if (savedAccounts) { | ||
accounts = JSON.parse(savedAccounts); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the structure of what you retrieve here, it might be modified to an undesired value. if this returns a non array value ,then your code with fail in line 16
src/utils/savedAccounts.js
Outdated
}; | ||
|
||
export const getLastActiveAccount = () => ( | ||
getSavedAccounts()[localStorage.getItem('lastActiveAccountIndex') || 0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if:
- Saved account is an empty array?
- lastActiveAccountIndex is modified to a non numeric string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then getLastActiveAccount
returns undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, thank you 348f38e fixed it.
getSavedAccounts()[localStorage.getItem('lastActiveAccountIndex') || 0] | ||
); | ||
|
||
export const setLastActiveAccount = ({ publicKey, network, address }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By convention, methods which set a value, return the new state.
same goes for setSavedAccount
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
src/components/login/login.js
Outdated
@@ -127,8 +127,9 @@ class Login extends React.Component { | |||
|
|||
autoLogin() { | |||
const { savedAccounts } = this.props; | |||
if (savedAccounts && savedAccounts.length > 0 && !this.props.account.afterLogout) { | |||
this.account = savedAccounts[0]; | |||
console.log('dass', savedAccounts); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove
7649b1e
to
93e3be3
Compare
cb7ce36
to
72ca0b9
Compare
...because it doesn't work on Jenkins
What was the problem?
There was no tool for managing multiple saved accounts.
How did I fix it?
By creating a component that can:
How to test it?
Review checklist