Sort and Filter indicators for the Experiments Table #1760

wolmir · 2022-05-24T14:22:14Z

Solves (#1708)

This PR adds sorting and filtering indicators in the form of icons to the experiments table headers.
It also changes the direction of the ellipsis truncation to left-side only and reduces the font-size of the leaf headers.

Specs

Figma

Resulting

Screen.Recording.2022-05-24.at.11.20.51.mov

maxagin · 2022-05-24T14:30:30Z

@wolmir great job! Just a few comments:

Let's have the arrow on the left side for the right aligned cell content and on right side for the left aligned (timestamp)
Can we use an arrow/symbol from the SORT BY section?

Please let me know if you need an example. Thank you !

Super quick draft:

maxagin · 2022-05-24T14:56:47Z

I have built a quick spec . Please see here and let me know if you have any comments.

maxagin · 2022-05-24T15:21:34Z

@wolmir no animation when toggling. Thank you !

rogermparent

Agreed with @maxagin's comments, but even as it is this is a good improvement!

shcheklein · 2022-05-24T23:37:02Z

@wolmir good stuff, thanks! 🙏

maxagin · 2022-05-25T05:53:29Z

Hey @wolmir, just wanted to share with you my arguments in relation to the initial comments, sorry I was in rush to share feedback before you push :) , so will do it now:

Arrow on the left side for the right aligned cell content …

The main reason is that, when the symbol is on the right side (for right align content) it will push the text, which will create unwanted movement and the focus will be also on the text instead of just indicator, when the label is actually not changing.

Use an arrow/symbol from the SORT BY section

The symbol ↑ is more compact and better describes the direction. Same time the caret icon arrow is typically meant to indicate that an accordion will open.

No animation when toggling

It is natural to have animation for the caret icon arrow and this is exactly what you have created! However for the direction arrow, spin animation won’t really work. The direction arrow can use linear animation instead.

wolmir · 2022-05-25T14:20:45Z

Hey @wolmir, just wanted to share with you my arguments in relation to the initial comments, sorry I was in rush to share feedback before you push :) , so will do it now:

Arrow on the left side for the right aligned cell content …

The main reason is that, when the symbol is on the right side (for right align content) it will push the text, which will create unwanted movement and the focus will be also on the text instead of just indicator, when the label is actually not changing.

Use an arrow/symbol from the SORT BY section

The symbol ↑ is more compact and better describes the direction. Same time the caret icon arrow is typically meant to indicate that an accordion will open.

No animation when toggling

It is natural to have animation for the caret icon arrow and this is exactly what you have created! However for the direction arrow, spin animation won’t really work. The direction arrow can use linear animation instead.

Thanks @maxagin !

mattseddon

Thanks for doing this. I have left some feedback on the code. Definitely need to fix the on resize issue before we merge. Also need some. design feedback re: the icon not being visible.

mattseddon · 2022-05-30T22:50:00Z

webview/package.json

@@ -24,6 +24,7 @@
  "dependencies": {
    "@tippyjs/react": "^4.2.6",
    "@vscode/webview-ui-toolkit": "^1.0.0",
+    "@vscode/codicons": "^0.0.30",


[C] We now have this as a dependency here and as a dev dependency in the root package.

mattseddon · 2022-05-30T22:55:38Z

webview/src/experiments/components/Experiments.tsx

-const DEFAULT_COLUMN_WIDTH = 75
-const MINIMUM_COLUMN_WIDTH = 50
+const DEFAULT_COLUMN_WIDTH = 100
+const MINIMUM_COLUMN_WIDTH = 90


[I] There are a couple of issues here.

The icon can still be hidden (not sure what to do about this cc @maxagin).

Screen.Recording.2022-05-31.at.8.54.34.am.mov

On resize is causing the sort direction to change.

Screen.Recording.2022-05-31.at.8.52.01.am.mov

For 1. That is one of the reasons that the old style was so effective.

[Q] Can we revert to the previous style so that we can get this in?

@mattseddon By previous style do you mean the top and bottom borders?

Yes. More of a question for @maxagin & @shcheklein.

Something like this @mattseddon @shcheklein

Screen.Recording.2022-06-02.at.18.46.07.mov

@wolmir got it, I like it! :)

the whole column header should clickable though, right, I don't have to click the ... symbol?

Yes, that's just for show

mattseddon · 2022-05-30T23:01:48Z

webview/src/experiments/components/table/TableHeader.tsx

+      className={cx(
+        styles.sortIcon,
+        `codicon codicon-${
+          sortOrder === SortOrder.ASCENDING ? 'arrow-up' : 'arrow-down'


[C] There is some history here. The first time we came to add icons, @rogermparent, @sroy3 and myself all added them at roughly the same time. We came up with three different implementations. Which we then standardised to the current SVGR model. From talking to @sroy3 I think we did this because we had custom SVGs that we wanted to add to the webview(s). As this may no longer be the case we may want to change the approach but we will need to reflect that single method throughout the webview code. Please raise an issue to discuss and we can talk about it in the retro at the end of the week.

I created one here: #1802

It could be a good thing to change it to classes later, it would be simpler to add new ones. Right now though, of all the "custom" icons, there is still the clock one that does not have its equivalent in codicons. Since the up and down arrows are already in the current implementation, changing this to use <Icon icon={sortOrder === SortOrder.ASCENDING ? AllIcons.UP_ARROW : AllIcons.DOWN_ARROW} /> should not be too complicated. We can review using the whole library later and use #1802 to decide what to do with the missing icon.

rogermparent · 2022-06-07T18:44:14Z

I see there was recently some code added for filter indicators. It's great that you're tackling this, but I think it would be best to keep it in a separate, even if chained, PR considering this one is big and active enough as is.

webview/src/experiments/components/table/TableHeader.tsx

mattseddon · 2022-06-09T02:58:18Z

Everything seems to be working but I am now having issues with hot-reload of the table code in the webpack-dev-server:

Closing the webview, restarting the server and reloading the webview solves this issue. Looking into it now.

mattseddon · 2022-06-09T03:43:08Z

Everything seems to be working but I am now having issues with hotreload of the table code in the webpack-dev-server:

Closing the webview, restarting the server and reloading the webview solves this issue. Looking into it now.

Seems a bit fixed:

Screen.Recording.2022-06-09.at.1.41.24.pm.mov

Let's keep an eye on it and adjust accordingly.

sroy3 · 2022-06-09T18:55:29Z

webview/src/experiments/components/table/TableHeader.tsx

+export enum SortOrderLabel {
+  ASCENDING = 'Sort Ascending',
+  DESCENDING = 'Sort Descending',
+  NONE = 'Remove Sort'


I think these could just go inside SortOrder instead and that would be one less enum.

sroy3 · 2022-06-09T18:57:06Z

webview/src/experiments/components/table/TableHeader.tsx

+      content={menuContent}
+      disabled={menuDisabled || menuSuppressed}
+      onShow={() => {
+        return !column.isResizing


Does this really need to be in a function?

No, It was left over from when I was trying to make the left-click sort work. I believe it's not needed anymore

sroy3 · 2022-06-09T19:05:44Z

webview/src/experiments/components/table/TableHeader.tsx

+  {
+    hidden: !hasFilter,
+    icon: AllIcons.LINES,
+    onClick: () => {},


Maybe we should set a property to have simple icons without actions. Currently there is a pointer cursor on the filter icon and it makes it seem as if clicking on it was going to do something and nothing happens.

sroy3 · 2022-06-09T19:09:43Z

webview/src/experiments/components/table/TableHeader.tsx

+                  type: MessageFromWebviewType.REMOVE_COLUMN_SORT
+                }
+              }
+            ]}


Should we always show all options? If there is currently no sort, do we need to show "Remove Sort"? Same for when sorted ascending or descending. It does look fine now, I'm just genuinely curious about whether or not it'd be better to remove or keep options.

rogermparent · 2022-06-09T19:25:27Z

I might be missing something, but nothing happens when I left-click a table header.

left-click-sort-not-working-demo.mp4

wolmir · 2022-06-09T19:27:15Z

I might be missing something, but nothing happens when I left-click a table header.

left-click-sort-not-working-demo.mp4

Yes, that was scrapped for the moment. I'll update the PR title in a moment

codeclimate · 2022-06-09T19:43:42Z

Code Climate has analyzed commit abeb4b8 and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1
Duplication	2

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.6% (0.0% change).

View more on Code Climate.

rogermparent

Sorry for the confusion! As far as adding indicators, I think this PR definitely fits the bill. Anything I could point out has already been pointed out before, but with the refactor of unnecessary onClick handlers I think we've crossed the threshold of mergeable.

* Add arrow sort indicators and filter indicators to columns * Fix filters prop dependencies * fix tests * add filters to deeply nested storybook * refactor table head for complexity * fix dev server chaos * Fix PR change requests * Remove unnecessary click handlers * Fix test imports Co-authored-by: Matt Seddon <mattseddon@hotmail.com>

wolmir requested review from rogermparent, mattseddon, sroy3 and maxagin and removed request for rogermparent May 24, 2022 14:22

wolmir self-assigned this May 24, 2022

maxagin self-assigned this May 24, 2022

wolmir force-pushed the exp-table-sort-by-left-click branch from 8ea80ba to a8e2eb3 Compare May 24, 2022 14:54

sroy3 approved these changes May 24, 2022

View reviewed changes

wolmir force-pushed the exp-table-sort-by-left-click branch from a8e2eb3 to d0f37af Compare May 24, 2022 15:23

maxagin removed their assignment May 24, 2022

rogermparent approved these changes May 24, 2022

View reviewed changes

mattseddon added the product PR that affects product label May 24, 2022

wolmir force-pushed the exp-table-sort-by-left-click branch from bffb6b1 to 94f1411 Compare May 30, 2022 17:48

mattseddon approved these changes May 30, 2022

View reviewed changes

wolmir mentioned this pull request May 31, 2022

Decide on a standard implementation for icons #1802

Closed

mattseddon mentioned this pull request Jun 1, 2022

Reconsider column names truncation rules and default width settings #1786

Closed

8 tasks

wolmir force-pushed the exp-table-sort-by-left-click branch 2 times, most recently from 959c2f7 to def7186 Compare June 2, 2022 17:45

wolmir added 2 commits June 8, 2022 19:50

Add arrow sort indicators and filter indicators to columns

6d3e1d1

Fix filters prop dependencies

c018383

wolmir force-pushed the exp-table-sort-by-left-click branch from e0fc931 to c018383 Compare June 8, 2022 22:50

codeclimate bot reviewed Jun 9, 2022

View reviewed changes

webview/src/experiments/components/table/TableHeader.tsx Show resolved Hide resolved

webview/src/experiments/components/table/TableHeader.tsx Show resolved Hide resolved

mattseddon added 2 commits June 9, 2022 11:53

add filters to deeply nested storybook

5304e57

refactor table head for complexity

495aa91

mattseddon requested review from shcheklein, sroy3, mattseddon and rogermparent June 9, 2022 02:58

fix dev server chaos

985c3c9

mattseddon mentioned this pull request Jun 9, 2022

Add filtered counts to Filter By tree #1866

Merged

sroy3 reviewed Jun 9, 2022

View reviewed changes

sroy3 approved these changes Jun 9, 2022

View reviewed changes

wolmir changed the title ~~Left-click sorting in the experiment table headers~~ Sort and Filter indicators for the Experiments Table Jun 9, 2022

wolmir added 3 commits June 9, 2022 16:35

Fix PR change requests

a92ef06

Remove unnecessary click handlers

a313637

Fix test imports

abeb4b8

sroy3 approved these changes Jun 9, 2022

View reviewed changes

rogermparent approved these changes Jun 9, 2022

View reviewed changes

mattseddon approved these changes Jun 9, 2022

View reviewed changes

mattseddon merged commit 4684af2 into main Jun 9, 2022

mattseddon deleted the exp-table-sort-by-left-click branch June 9, 2022 22:03

mattseddon mentioned this pull request Jun 9, 2022

Experiments table: sort by left click on the header #1708

Closed

rogermparent mentioned this pull request Jun 15, 2022

Show an indication of filtering on the Experiment Table webview #1827

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort and Filter indicators for the Experiments Table #1760

Sort and Filter indicators for the Experiments Table #1760

wolmir commented May 24, 2022 •

edited

Loading

maxagin commented May 24, 2022 •

edited

Loading

maxagin commented May 24, 2022

maxagin commented May 24, 2022

rogermparent left a comment

shcheklein commented May 24, 2022

maxagin commented May 25, 2022

wolmir commented May 25, 2022

mattseddon left a comment

mattseddon May 30, 2022

mattseddon May 30, 2022

mattseddon May 30, 2022

mattseddon May 30, 2022

wolmir May 31, 2022

mattseddon May 31, 2022

wolmir Jun 2, 2022

shcheklein Jun 2, 2022

shcheklein Jun 2, 2022

wolmir Jun 2, 2022

wolmir Jun 2, 2022

mattseddon May 30, 2022

wolmir May 31, 2022

sroy3 May 31, 2022

rogermparent commented Jun 7, 2022

mattseddon commented Jun 9, 2022 •

edited

Loading

mattseddon commented Jun 9, 2022

sroy3 Jun 9, 2022

sroy3 Jun 9, 2022

wolmir Jun 9, 2022

sroy3 Jun 9, 2022

sroy3 Jun 9, 2022

rogermparent commented Jun 9, 2022

wolmir commented Jun 9, 2022

codeclimate bot commented Jun 9, 2022

rogermparent left a comment

Sort and Filter indicators for the Experiments Table #1760

Sort and Filter indicators for the Experiments Table #1760

Conversation

wolmir commented May 24, 2022 • edited Loading

Specs

Resulting

maxagin commented May 24, 2022 • edited Loading

maxagin commented May 24, 2022

maxagin commented May 24, 2022

rogermparent left a comment

Choose a reason for hiding this comment

shcheklein commented May 24, 2022

maxagin commented May 25, 2022

wolmir commented May 25, 2022

mattseddon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogermparent commented Jun 7, 2022

mattseddon commented Jun 9, 2022 • edited Loading

mattseddon commented Jun 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogermparent commented Jun 9, 2022

wolmir commented Jun 9, 2022

codeclimate bot commented Jun 9, 2022

rogermparent left a comment

Choose a reason for hiding this comment

wolmir commented May 24, 2022 •

edited

Loading

maxagin commented May 24, 2022 •

edited

Loading

mattseddon commented Jun 9, 2022 •

edited

Loading