Migrate Table visualization to React Part 1: Renderer

[kravets-levko]

What type of PR is this? (check all applicable)

• Refactor

Description

• Replace <dynamic-table> with Ant's <Table> component
• Implement custom features:
  • number, date/time, boolean formatting;
  • HTML features (simple formatting, highlight links);
  • Image column;
  • Link column;
  • JSON column;
  • search in data;
  • sort data by column click (multi-column sort);
• Cleanup
• Tests

Related Tickets & Documents

#3301 (Migrate Visualizations to React -> Table)

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Mostly the same as before:

[kravets-levko]

@arikfr @ranbena @gabrieldutra I still need to create some tests for Table visualization, but this code is ready for review. Also, someone who had that issue with horizontal scroll - please test it again with new component (I removed that CSS because entire <dynamic-table> was replaced, but I may need to restore it if that bug reproduces with Ant's Table as well). Thanks!

[arikfr]

@kravets-levko considering that Ant takes care of the indicators in new version, let's keep as is. 👍

[ranbena]

@ranbena when you move the cursor out, does it hide the scrollbar?

@arikfr usually yes, but after numerous attempts it eventually sticks.

[ranbena]

Hold up! This might have been fixed finally for Chrome 76 https://bugs.chromium.org/p/chromium/issues/detail?id=914844#c77

[ranbena]

@kravets-levko perhaps upgrade Ant so the new table header design kicks in?

[ranbena]

Hold up! This might have been fixed finally for Chrome 76 https://bugs.chromium.org/p/chromium/issues/detail?id=914844#c77

Just upgraded and can't repro bug 👍

[ranbena]

Just upgraded and can't repro bug 👍

Nope, I just did 👎

[kravets-levko]

perhaps upgrade Ant so the new table header design kicks in?

Already doing (need to resolve conflicts as well).

Just upgraded and can't repro bug 👍

Nope, I just did 👎

You reproduced the bug in Chrome 76?

[ranbena]

You reproduced the bug in Chrome 76?

Yes but I just realized I got v76.0.3809 and the fix is on v76.0.3901..

[kravets-levko]

@ranbena

P.S. Ant's Table is a bit stupid: it has built-in sorting feature, which supports single-column sort. It's also possible to disable it (sorter: true and then set sortOrder per column where needed) - table will just show sorter icons and will not actually sort data. But it does not allow to display sort order on multiple columns - always only one column can have active (blue) arrow, others will be ignored 🤦‍♂️

[ranbena]

@kravets-levko it looks great!

Regarding the cell height, I prefer the original 33px only for the sake of backwards compatibility, if you think it's important.

Regarding the th ellipsis we chose max-width: 100px but it does have a downside - it'll get clipped even if there's room for the whole title.

1. Perhaps consider a higher max value so only absurdly long titles get clipped.
2. In a following PR, allow users to edit viz setting with full/clipped/multi-row.

[kravets-levko]

@ranbena

Regarding the cell height, I prefer the original 33px only for the sake of backwards compatibility, if you think it's important.

I'm not sure it's so important. Auto-height feature exists to deal with variable-height tables, and it may mess up dashboard layout any time (not only after CSS changes, but also after re-running query, changing visualization options, etc.). Considering this, I prefer to not override line-height for the visualization.

1. Totally agree. What could be a better value for this - how do you think? I think 200px should be more than enough, but who knows 🤔
2. I think it's better to create an issue (you can even assign it to me, or label as "Help wanted").

[ranbena]

1. Totally agree. What could be a better value for this - how do you think? I think 200px should be more than enough, but who knows 🤔

200 is better than 100 in this case 👍 and having the setting later will make sure everyone's happy.

2. I think it's better to create an issue (you can even assign it to me, or label as "Help wanted").

Will do.

[kravets-levko]

@arikfr @gabrieldutra If everyone is happy with this PR - I'll merge it

[arikfr]

👍