Upgrade react-table

[sroy3]

Closes #2130
Will also fix #2307

Hopefully no use cases were missed.

In the end, if you look at the diff size, the upgrade should be very beneficial code-wise.

[codeclimate]

Function buildColumns has a Cognitive Complexity of 9 (exceeds 5 allowed). Consider refactoring.

[shcheklein]

Issues I've found so far:

• shadow in the first column appears after you open the table first time
• borders are not stable when you scroll

Screen.Recording.2023-01-19.at.5.40.44.PM.mov

• Fonts look different to me (e.g. header - bold vs non bold)

• Created column layout is different (dates take too much space atm)

• Cell borders looks different on row hover in selected and non-selected state

• When there is a single row of in the header it becomes a mess:

[shcheklein]

@sroy3 @julieg18 Q: I thought we had counts on collapsed chevrons for experiments - I don't see them now (in this version and in the production one) - am I missing something or is it a regression?

[sroy3]

@sroy3 @julieg18 Q: I thought we had counts on collapsed chevrons for experiments - I don't see them now (in this version and in the production one) - am I missing something or is it a regression?

Regression, I did not notice them. I'll add them. There is no count on chevrons. There was a count in the code, but it's not used in that way. There are counts for "selected", "starred" and "selected for plotting".

[sroy3]

🤞🏼 Everything is fixed. Will have another fresh look tomorrow, because I think I might be losing the big picture right now at the end of the day. Feel free to recheck or to wait until I have taken a second look tomorrow.

[codeclimate]

Function Cell has 34 lines of code (exceeds 30 allowed). Consider refactoring.

[shcheklein]

Thanks, @sroy3 ! I think everything reported above is fixed. Do we plan to review and fix chromatic tests?

One more thing that I found (the first column width jumps):

Screen.Recording.2023-01-23.at.6.37.14.PM.mov

Btw, on the resizer. I think it makes sense from the product perspective to show them separate in some cases:

Screen.Recording.2023-01-23.at.6.34.50.PM.mov

They have a bit different semantics. The group one can change width evenly underneath it. While the cell one at the bottom can change only that cell width. It's not exactly consistent at the moment, but it might be easier to fix it to work that way. WDYT?

[sroy3]

Btw, on the resizer. I think it makes sense from the product perspective to show them separate in some cases:

Screen.Recording.2023-01-23.at.6.34.50.PM.mov
They have a bit different semantics. The group one can change width evenly underneath it. While the cell one at the bottom can change only that cell width. It's not exactly consistent at the moment, but it might be easier to fix it to work that way. WDYT?

How about resizer lines span all the way to the top (to ancestor columns), but they do not span down to descendant and leaf columns)? I think I can do that and it makes the most sense. If you're resizing a leaf column, the parent columns also resize accordingly. If we resize a parent column, the new size applies to all child columns.

This is what this looks like:

Screen.Recording.2023-01-24.at.10.07.08.AM.mov

[codeclimate]

Function ColumnResizer has a Cognitive Complexity of 9 (exceeds 5 allowed). Consider refactoring.

[sroy3]

I went over pretty much everything this morning and everything is pretty much as it was without any border or misalignment problems. We can see from storybook that the biggest difference is in the placeholders (ancestor column vs leaf column). This is a difference inside of react-table itself and there isn't much we can do about it. Other than that, things move a little bit, but this is expected when changing the layout so much.

[shcheklein]

@sroy3

This is what this looks like:

Idea makes total sense to me. Let's do it that way. Not a blocker for merging this if it's approved from the code perspective by @julieg18 and/or @mattseddon .

From the video though, I see that it's a bit inconsistent still, right? E.g. when you move the step cell the resizer should snap only that header cell, right? Not the whole height.

[sroy3]

@sroy3

This is what this looks like:

Idea makes total sense to me. Let's do it that way. Not a blocker for merging this if it's approved from the code perspective by @julieg18 and/or @mattseddon .

From the video though, I see that it's a bit inconsistent still, right? E.g. when you move the step cell the resizer should snap only that header cell, right? Not the whole height.

It also resizes the training/metrics.json cell though (we cannot really change that behaviour as it is the last column for training/metrics.json. Rules are whatever the level, it spans to the top of the table, but it ends with the column (it won't go further down).

[shcheklein]

It also resizes the training/metrics.json cell though (we cannot really change that behaviour as it is the last column for training/metrics.json. Rules are whatever the level, it spans to the top of the table, but it ends with the column (it won't go further down).

Okay, makes sense. Let's proceed with this!

[codeclimate]

Code Climate has analyzed commit cdd01cc and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1

The test coverage on the diff in this pull request is 93.1% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.0% (-0.2% change).

View more on Code Climate.