Conversation
Codecov Report
@@ Coverage Diff @@
## master #20 +/- ##
==========================================
+ Coverage 96.87% 97.35% +0.48%
==========================================
Files 4 4
Lines 224 227 +3
Branches 52 54 +2
==========================================
+ Hits 217 221 +4
+ Misses 7 6 -1
Continue to review full report at Codecov.
|
| isAscending = column.sorted.asc; | ||
| // if it's undefined, start by setting to ascending, otherwise toggle | ||
| isAscending = | ||
| column.sorted.asc === undefined ? true : !column.sorted.asc; |
There was a problem hiding this comment.
If column.sorted.asc is already set to true, won't this set it to false? Seems like we should define column.sorted.asc as true/false earlier instead of letting it be undefined.
There was a problem hiding this comment.
Yeah because this is the toggle sort action (like when you click the header), so we want it to go from true -> false and false->true.
In the case that it's undefined (the default), we just want to start with ascending order, if that makes sense. Undefined marks the state before any sorting has happened. If you think this is more confusing I can set it to true/false only again.
There was a problem hiding this comment.
Ah my mistake, I didn't realize we were in the TOGGLE_SORT case. I generally think it's better to not leave things undefined, but it's your call.
There was a problem hiding this comment.
Yeah I generally agree, although here I'm not setting things to undefined explicitly. Just having a default "not set" state. I guess I could make it null | boolean which might be slightly better.
3 participants
No description provided.