Riaan Onboard Project Review #50
Conversation
riaanwastaken
requested review from
AshtonMar,
CelesteNaude,
DevinFledermaus,
FritzOnFire and
anzelIMQS
FritzOnFire
left a comment
FritzOnFire
left a comment
There was a problem hiding this comment.
Please use tab's, not spaces.
FritzOnFire
left a comment
FritzOnFire
left a comment
There was a problem hiding this comment.
This was a mixture of high level and in dept review... I will have to review again after these changes, as I feel like I mostly focused on if you are always passing up your errors as high as possible before handling them. I may have asked you to remove a try-catch where you actually need one. I would recommend asking me tomorrow if you feel that I may have miss under stood some of your code, and that you definitely need need a try-catch in some function.
But, aside from all that. WOW... wow... this code... it is so clean, so easy to read. Covers so many bases, checking all cases and corners. Its really really good, and is the type of stuff we ask for in every PR. Usually code only looks like this after several people have reviewed it and it has gone through a few reviews (I am not referring to the onboarding project, I am referring to our normal everyday code). You have set the bar really really high :) This is good stuff :)
views.ts
Outdated
|
|
||
| tbody.innerHTML = ""; | ||
|
|
||
| records.forEach((record) => { |
There was a problem hiding this comment.
Please do not use forEach. https://github.com/IMQS/www-lib/tree/master/_codestyle#loops
There was a problem hiding this comment.
@FritzOnFire apologies, I thought I handled this. Is it because there is less control over forEach that it is not recommended ?
2 participants
Dear Reviewer(s),
For the best experience, please start at "app.ts" for a brief project overview.
Thank you for your time.