-
Notifications
You must be signed in to change notification settings - Fork 195
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
React Router v6 + Auth Flow #384
Conversation
dylancom
commented
Nov 15, 2021
•
edited
Loading
edited
- Upgraded from v5. (Many API changes)
- Added auth-flow (using context). Protected screens will not be viewable anymore when locked.
No matching routes for adding a new account from the options page:
|
Fixed |
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.
I can't say much about the code and it is hard to compare. So far it works great, I will keep testing it more and we should soon merge this to prevent further merge conflicts.
can you maybe rebase this one?
src/app/context/AuthContext.tsx
Outdated
|
||
const unlock = (password: string, callback: VoidFunction) => { | ||
return utils.call("unlock", { password }).then(() => { | ||
setUser("alby"); |
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 we call setUser with "alby"? - can we document that somewhere?
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.
I am wondering if calling it user
is the best here... we will always only have one user, but multiple accounts. maybe we can "authenticate" towards one account? and store here which account is currently used?
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.
Currently "alby" is just a placeholder, could be "user" aswel. We only check for the presence of a value inside the app. But we could store here also which account is currently used.
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.
if we call it account or something like currentAccount then we would not introduce another terminology/entity.
…lightning-browser-extension into upgrade/react-router-v6
…lightning-browser-extension into upgrade/react-router-v6
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.
had some minor comments about some debug logs that we can remove probably?
Let's merge it and then test the app in detail again. @MoritzKa can you also do another testing round
@dylancom can you rebase and then merge? |
Co-authored-by: Michael Bumann <hello@michaelbumann.com>
Co-authored-by: Michael Bumann <hello@michaelbumann.com>
Co-authored-by: Michael Bumann <hello@michaelbumann.com>