Show All Animals on Manage Animals Page #1482
Conversation
|
Openshift URLs for the PR Deployment: |
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1482 +/- ##
==========================================
- Coverage 47.56% 46.90% -0.66%
==========================================
Files 930 921 -9
Lines 25641 23818 -1823
Branches 3954 3540 -414
==========================================
- Hits 12195 11173 -1022
+ Misses 12794 12044 -750
+ Partials 652 601 -51 ☔ View full report in Codecov by Sentry. |
| if (critterIds.length) { | ||
| geometryDataLoader.load(critterIds); |
There was a problem hiding this comment.
(No change needed, just some context around the way this was written before, with the negative length check and early return)
In general, It is considered best practice to keep code as flat as possible (with as little nesting as possible, when its reasonable to do so).
In this case, we have to do the if check for the length no matter what, but it is slightly nicer to not nest 'real' code when possible. Hence writing it like this:
if (what_we_dont_want) {
return; // end early
}
if (also_what_we_dont_want) {
return; // end early
}
return what_we_do_want();
In a similar vein:
if (some_condition) {
return 1;
} else {
return 2;
}
is better written as:
if (some_condition) {
return 1;
}
return 2;
Since the else is implied, we can remove that extra level of nesting and reduce how many brackets you have to parse to understand the code.
In this case, the useEffect has so little code that no one is seriously going to worry about it. Just more of a heads up for future, especially if you have more than one if/else statement.
There was a problem hiding this comment.
Hi! Thank you so much for taking the time to provide insightful, detailed feedback! I will review and revise :)
| */ | ||
| staticLayers: IStaticLayer[]; | ||
| interface ISurveySpatialAnimalMapProps { | ||
| staticLayers?: IStaticLayer[]; |
There was a problem hiding this comment.
Can we add the JSDoc back?
There was a problem hiding this comment.
More documentation is always a win
| } | ||
|
|
||
| geometryDataLoader.load(critterIds); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [critterIds]); |
There was a problem hiding this comment.
Missing dependency: geometryDataLoader
There was a problem hiding this comment.
It is quite possible that including that dependency will result in an infinite loop of requests. In which case we need to include that comment // eslint-disable-next-line react-hooks/exhaustive-deps which tells the linter (eslint) to ignore the fact that there is a missing dependency.
There was a problem hiding this comment.
In this case it may be fine to include it, as the load function is designed to only run once. So even if the useEffect is triggered more than once, the load function internally will only actually make the request one time ever.
mauberti-bc
added
Early Feedback Welcome
mauberti-bc
changed the title
Description of Changes