-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
Simplify and explain login flow #2317
Conversation
group-income Run #3100
Run Properties:
|
Project |
group-income
|
Branch Review |
2271-simplify-login-flow
|
Run status |
Passed #3100
|
Run duration | 09m 36s |
Commit |
0cdc4d2044 ℹ️: Merge 20f94273df2b9e72cdc2d8ac544ad4f0d57a3596 into e341e652a02766143f1a084f3547...
|
Committer | Ricardo Iván Vieitez Parra |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
10
|
Skipped |
0
|
Passing |
111
|
View all changes introduced in this branch ↗︎ |
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.
Question: is this PR finished or is there more work to do?
I ask because as far as the jumping around between events related to login that happens when following the logic of actions/identity.js
and app/identity.js
, that seems unchained. Is there no way to simplify that part / make it clearer?
As mentioned on Slack, the only thing missing (other than feedback) is completing the
I'm going to assume you mean unchanged other than unchained. Even then, I'm still unsure what you're asking specifically. If you're talking about where the event listeners are defined, they are in those files as per your request when those event listeners were defined. There's no particular reason why they couldn't be in a different location. If you're talking about the events themselves, that's the result of the service worker and the app being different contexts entirely and of both needing to know about the currently logged in user. |
Yes, that was a typo. This PR does a great job of simplifying What I'm wondering is, is it possible in any way at all, to simplify the logic that's inside of |
I've taken a look and it looks pretty simple to me already (both about 100 lines with relatively straightforward logic). I'll add some brief additional comments to the uncommented sections. |
I consider this PR complete now unless there are additional requests. I highly recommend taking the time to read through |
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.
Excellent work on LOGIN_FLOW.md
! Left some requests for changes & typo fixes.
Thanks! Updated as requested. |
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.
Alright @corrideat - this PR is quite something. I tested it, and it seems to work. The tests pass. Let's cross our fingers nothing sneaky broke. 🤞
No description provided.