Fix and standardize multiple table UI inconsistencies

[alexkim205]

Changes

Wrapped up a bunch of table fixes in this PR.

• Fixes Table Overflow Revamp #4725 (Master) Table Overflow Revamp
• Fixes Feature flags table formatted wrong on the first load #4500 Feature flags table formatted wrong on the first load
• Fixes Squishy Tables #4721 Squishy tables
• Fixes Actions Table Overflow inaccessible on smaller screens #4723 Actions Table Overflow inaccessible on smaller screens
• Fixes Inconsistent scroll shading for fully visible table #4724 Inconsistent scroll shading for fully visible table

A bunch of after pics (in no particular order)

Screen.Recording.2021-06-11.at.4.40.06.PM.mov

actions.mov

Checklist

• All querysets/queries filter by Organization, by Team, and by User
• Django backend tests
• Jest frontend tests
• Cypress end-to-end tests
• Migrations are safe to run at scale (e.g. PostHog Cloud) – present proof if not obvious
• Frontend/CSS is usable at 320px (iPhone SE) and decent at 360px (most phones)

[mariusandra]

I simplified the breakpoint code a bit, yet still found several issues in the app that I'll gladly pass on :)

1. Feature flags do not display well, no matter how I open the page. The key is always cut for me, and the description area is needlessly huge:
   

2. Events page (zoomed out to simulate a wide screen) won't let me resize things like I expect:
   

3. No "you can scroll right" indicator here:
   

Probably outside the scope, or maybe not:

4. If the widths auto-update while resizing, my dragging cursor is "let go" and I need to start resizing again:

5. There's no way to make the last column of any table wider or shorter, plus the whitespace that's at the end of the table:
   

---

PS, my most common PR review comment is "simplify!", so feel free to always do that automatically :D. The logic here is that code is not written for computers to read, but for other humans to read. Computers can read your code no matter how you well you write it, but the human reading it will be caught off guard by clever code that's hard to parse. So always optimise code clarity for the cognitively lowest life form in the process: your fellow humans. Usually this means "make your code so simple it looks like a first year student wrote it".

[mariusandra]

The breaking cypress tests seem relevant as well.

[mariusandra]

One more bug, this time in this cohorts table:

[paolodamico]

Looks like @mariusandra already did a comprehensive review of this, tag me for a follow up review if needed @alexkim205

[samwinslow]

2. Events page (zoomed out to simulate a wide screen) won't let me resize things like I expect:

Fixed this by removing the arbitrary max column width I had set by default.

1. There's no way to make the last column of any table wider or shorter, plus the whitespace that's at the end of the table:

This one gets tricky because in order for the table to render correctly, one column needs to have an uncontrolled width set by the browser. But, because the size of the entire table and other columns update together, it works out such that this "uncontrolled" column appears to have a fixed width. Right now the last column is chosen for this unfortunate caveat, but maybe we could always push a blank column onto the end of the ones given to us via props.

[mariusandra]

In order for the table to render correctly, one column needs to have an uncontrolled width set by the browser.

Why? :)

[samwinslow]

@mariusandra If every column has a set width, the browser will just decide what it thinks appropriate width values are in order to fill the horizontal space of the <table>, when using default table-layout... Antd's tableLayout: fixed prop did not seem to change this behavior but more experimentation could be done.

Antd's docs have a similar warning:

Specify width of columns if header and cell do not align properly. If specified width is not working or have gutter between columns, please try to leave one column at least without width to fit fluid layout, or make sure no long word to break table layout.

When I was working on ResizableTable it was very hard to keep the resizable column headers in sync with the table's actual columns. I don't mean to suggest it's impossible but it might get hacky!

[mariusandra]

Yeah, this sounds tricky. I don't know the nitty gritty details of antd's tables, and those tradeoffs probably make sense.

Yet it's funny how "resizable table headers" is still an unsolved computer science problem :).

Can't we just, after the table has fully loaded, capture all the flexibly set column widths, and then just keep them all in a controlled state as we start to resize? Not sure if this helps anything 😅

[alexkim205]

Thanks for the input everyone. Given the above discussion on ResizableTable, there're two more things I want to look into in this PR.

1. Bug fixes (thanks @mariusandra for the comprehensive dive)
2. Refactoring + Simplifying <ResizableTable/> (which fixes some bugs in 1.)

Please correct me if I'm wrong, but from my interpretation of today's code, we nix antd's table header, and prepend a custom header with columns that we keep in sync with the table's columns. If the above reading of the code is correct, this seems to bring a lot more pain than joy when dealing with edge cases like:

• filling out the last column width
• table scrolling independently from headers
• inconsistent scroll gradient
• more scroll handlers
• and more!

Have we previously considered the resizable columns implementation in the official antd documentation? If I'm not overlooking an obvious disadvantage with this option, I feel passing in our header.cell component instead of completely replacing the component and handling column sizing in state ourselves is a lot cleaner and easier to maintain.

---

More on point 2: I'll put out a proof of concept about what I mean soon

[samwinslow]

@alexkim205 The example in the Antd docs is a bit of a red herring. It's not an obvious disadvantage at first — it re-renders the entire table on each resize event, which happens every few milliseconds. It gets borked on very long tables like Events or Sessions. Even if the resizes are throttled to occur once every 60 or 100 ms, e.g. with requestAnimationFrame, rendering may still take longer, and a 100ms interval works out to about 10 frames per second — looks quite bad.

This actually was my first approach, but then I settled on a CSS-based solution; updating the widths specified in <colgroup> styles does not trigger a React render, but only a browser repaint.

[alexkim205]

Discussed some possible approaches during standup. Here are some notes for posterity:

• passing in columns prop with width makes antd's <Table> component rerender aggressively
• Virtual table header is a good solution but introduces several bugs like the one this PR tries to address
• Maybe see if we can use other table libraries that can better handle of large amounts of data
  • react-table - CodeSandbox doesn't handle many rows well either.
  • react-virtualized - CodeSandbox is promising.
  • react-window - smaller lightweight version of react-virtualized
  • react-table + react-window - highly extensible way to use hooks and custom ui components to bring expandable, virtualization, resizable features to existing table

[mariusandra]

react-table looks pretty awesome! Unless I'm terribly wrong, it shouldn't be that hard to implement it and make our tables look like they do now? :)

[alexkim205]

Yup it's a headless utility library so it's pretty cool that you can pick and choose features you want to include. One feature I've been having a hard time playing nicely is expandability. Still trying to figure out a way to make rows expandable without breaking resizability and virtualization

In summary, want to combine

• https://react-table.tanstack.com/docs/examples/virtualized-rows
• https://react-table.tanstack.com/docs/examples/expanding
• https://react-table.tanstack.com/docs/examples/full-width-resizable-table <--- example is broken on their website

[alexkim205]

1. Feature flags do not display well, no matter how I open the page. The key is always cut for me, and the description area is...

1. Addressed bug 1 by ellipsifying key and description columns (the two most variable columns) for more consistent row heights. Added tooltips so that user can still view any text that is cutoff.
2. Fixed failing cypress tests with more specific selectors
3. Fixed column widths of action columns (Enabled, Usage, Actions) so that we can have more space for the other columns and fixed them to the right.

Demo

feat.mov

Bugs 2-5 happen on the events table and I think it requires a different implementation of the events table to fix this. I'll address this in another PR as it's out of the scope.

[alexkim205]

One more bug, this time in this cohorts table:

I think this bug is fixed by my commit here that was merged in as part of @paolodamico's InsightsTableV2 update. But the fix hasn't been made in V1, so I'll bring my commit over here too.

[alexkim205]

Whelp, should have merged instead of rebase from master. Please ignore all those commits that I brought in except the last one. I'll squash and clean up accordingly when I merge this PR in.

[mariusandra]

Uh... sorry, but I can't review this. Not sure how it came to be, but there are thousands of changed lines in 77 files:

Can you please clean it up? :)

[mariusandra]

.

[alexkim205]

Hey @mariusandra sorry about the commit cereal. I think this rev should be cleaner

[mariusandra]

I'm not sure how annoying this feedback is to fix, but it's all given in the service of "let's have better UX". I'm also not sure if any of my comments override anything said before or not. :)

Some comments inline, some below.

1. Let's remove the ellipsis from the feature flags table. I think There's no reason this table needs fixed heights. Looking at our own data, if we'd merge this, we'd have a sub-par experience... as we'll have mountains of whitespace all around the page, yet we won't be able to actually read any flag keys and descriptions. That sounds like a failure. So can we just make it look like this, but... uhm... work? :D

2. Feature flags don't tick the "320px" checkbox:
   

3. Similar comment for the annotations table. The annotation column is anyway in two lines sometimes if "last updated" pushes it so.... so why the ellipsis? Let's remove it. The whitespace is also very suboptimal in full screen.

[alexkim205]

Addressed a-c above and added some goodies where the table doesn't word break columns when entering scroll mode.

Annotations Table

Screen.Recording.2021-06-21.at.10.55.48.AM.mov

Feature Flags Table

Screen.Recording.2021-06-21.at.10.47.38.AM.mov

Word breaking for date-specific columns

P.S. Failing e2e tests don't seem related

addressed all change requests

[mariusandra]

Found (and fixed) just one weirdness with a double key for feature flags, probably from some merge:

Other than that, there's one new weirdness with the events table. Yet I'd suggest to alreadyfinally merge this and tackle this in a new issue.

[mariusandra]

Even got it to look like this one time:

[alexkim205]

Thanks for the hot fix, looks like a bad merge resolution on my part.

Yeah the events table (and other tables that use our custom ResizableTable component) is a bit funky and has a few bugs that I think should be addressed in another PR. react-table looks like the best approach so far for maintaining current table functionality while reducing antd gymnastics