- Notifications
You must be signed in to change notification settings - Fork928
refactor: Load users from the API#1218
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedApr 29, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #1218 +/- ##==========================================+ Coverage 65.45% 65.67% +0.21%========================================== Files 268 269 +1 Lines 17029 17338 +309 Branches 162 164 +2 ==========================================+ Hits 11147 11386 +239- Misses 4710 4764 +54- Partials 1172 1188 +16
Continue to review full report at Codecov.
|
| if (!users) { | ||
| return <FullScreenLoader /> | ||
| } |
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.
We avoid early return on the frontend (it's a newish decision)
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'm not sure what early return means... but in case there are no users, during the initial load, what should we do instead?
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.
Sorry, your content is fine, I just mean please useelse to connect the conditions.
BrunoQuaresmaApr 29, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Ahh, ok. I will do that but I'm curious about the reason.
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.
Sure - the idea is that usingelse makes it clear that the line of code you're reading is only executed conditionally. Withoutelse, the fact that the line is conditional is only represented by an apparently unrelatedif that happens to containreturn. So it's easier to misunderstand when maintaining the code.
Uh oh!
There was an error while loading.Please reload this page.
@Emyrk just confirming there's no pager |
@presleyp we removed the pagination on the backend for now. Here is the discussion:https://codercom.slack.com/archives/C01A9SEKFEE/p1651240046986249 |
Closes#1216