Show middle state badges for running, starred and checked sub-rows #2004

wolmir · 2022-07-08T12:23:57Z

This PR will introduce indicators for the state of sub-rows in the experiments table.

There will an indicator for selected, starred and running sub-rows.

See #1714 for context on the issue.

Screen.Recording.2022-07-18.at.10.34.09.mov

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

webview/src/shared/components/tooltip/Tooltip.tsx

mattseddon · 2022-07-11T22:16:38Z

[Q] Why are we not showing the middle state for experiments that have been selected for plotting?

wolmir · 2022-07-11T22:20:14Z

[Q] Why are we not showing the middle state for experiments that have been selected for plotting?

I missed it, but it's a relatively simple change now. I can include them in this PR

mattseddon · 2022-07-12T10:40:39Z

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

+          )} ${actionAppliedLabel}.`
+        }
+      >
+        {children}


[I] At the moment only the direct children are being aggregated. Here is a screen recording of an example:

Screen.Recording.2022-07-12.at.8.30.37.pm.mov

When we aggregate from checkpoints to experiments and then up to the current commit we are not taking the cumulative total into account.

Another example:

Screen.Recording.2022-07-12.at.8.35.31.pm.mov

I would not even try to include the top level indicator until we have more than one commit/branch in the table. Until that time we will be catering to an edge case.

However, if you feel that they are needed then they need to reflect the totals including all children.

More complicated aggregation behaviour will also need some tests.

I'll add some tests for this current use-case now, but I want to defer this total aggregation for later if it's currently an edge case.

webview/src/experiments/components/table/styles.module.scss

wolmir · 2022-07-12T14:43:55Z

I just added the plots indicator and will add some tests for all of them now

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

webview/src/test/tableDataFixture.ts

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

wolmir · 2022-07-12T21:19:47Z

This pull request will bring the total coverage in the repository to 96.6% (0.0% change).

It was 96.7% before I added more tests. Go figure

webview/src/experiments/components/App.test.tsx

wolmir · 2022-07-14T16:12:15Z

webview/src/stories/Table.stories.tsx

+  userEvent.click(collapseButtons()[1])
+  userEvent.click(collapseButtons()[2])
+  userEvent.click(collapseButtons()[3])
+}


I thought this warranted its own storybook story

wolmir · 2022-07-14T16:14:30Z

webview/src/test/tableDataFixture.ts

+    selected: true
+  }))
+}
+export const setExperimentsAsRunning = setRowPropertyAsTrue('running')


I added these helpers to remove some of the friction I had with adding new tests. Hopefully this will make it way easier to setup table data scenarios for new tests.

wolmir · 2022-07-14T16:15:37Z

webview/src/experiments/components/table/styles.module.scss

+          }
+        }
+
+        .bullet {


I need to remove this, they are not used anymore

Please remove this before merging

wolmir · 2022-07-14T16:18:06Z

webview/src/experiments/components/table/styles.module.scss

@@ -672,3 +714,26 @@ $badge-size: 0.85rem;
 .cellTooltip {
  padding: 2px 6px;
 }
+
+.indicatorCount {
+  &[aria-label='0'] {


I tried using only this style to not show the counter when the number is 0, because it would save a conditional test in the component and reduce its complexity.

It works on VSCode and Storybook, but for some reason these styles are not applied when running the jest tests.
So I kept the conditionals in the end.

wolmir · 2022-07-14T16:19:13Z

webview/src/stories/Table.stories.tsx

      subRows: experiment.subRows?.map(checkpoint => ({
        ...checkpoint,
-        running: checkpoint.running || checkpoint.label === '23250b3'
+        running: checkpoint.running || checkpoint.label === '23250b3',
+        starred: checkpoint.starred || checkpoint.label === '22e40e1'


I can simplify this by using the helpers I mentioned above

mattseddon

Just one thing that I would recommend doing before you merge this. Adding an updated video in description would be handy for future reference too.

mattseddon · 2022-07-15T01:53:17Z

webview/src/experiments/components/App.test.tsx

@@ -372,6 +373,129 @@ describe('App', () => {
    })
  })

+  describe('Sub-rows middle states indicators', () => {
+    const renderWithAllRowsExpanded = () => {


[I] A more central location for these util will be helpful.

Splitting out utils will help to start reducing the size of this file and would give a central location for people to look for test building blocks.

fine to create a separate PR for that

mattseddon · 2022-07-15T02:17:47Z

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

+      role={'marquee'}
+      aria-label={`${count ?? 0}`}
+    >
+      {count ?? 0}


[Q] How is ?? needed here when it can only be a truthy value?. Could you use a type guard here?

mattseddon · 2022-07-15T02:18:56Z

webview/src/experiments/components/table/styles.module.scss

+          }
+        }
+
+        .bullet {


Please remove this before merging

mattseddon · 2022-07-15T02:19:11Z

webview/src/stories/Table.stories.tsx

      subRows: experiment.subRows?.map(checkpoint => ({
        ...checkpoint,
-        running: checkpoint.running || checkpoint.label === '23250b3'
+        running: checkpoint.running || checkpoint.label === '23250b3',
+        starred: checkpoint.starred || checkpoint.label === '22e40e1'


Add some stars to the Table stories data

codeclimate · 2022-07-18T13:49:32Z

Code Climate has analyzed commit 84d777c and detected 0 issues on this pull request.

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

This pull request will bring the total coverage in the repository to 96.7% (0.0% change).

View more on Code Climate.

codeclimate bot reviewed Jul 8, 2022

View reviewed changes

webview/src/experiments/components/table/Cell.tsx Outdated Show resolved Hide resolved

webview/src/experiments/components/table/Cell.tsx Outdated Show resolved Hide resolved

wolmir marked this pull request as ready for review July 11, 2022 19:33

wolmir requested review from mattseddon, rogermparent and sroy3 as code owners July 11, 2022 19:33

wolmir mentioned this pull request Jul 11, 2022

Show middle state icon for parent Runs, Checkboxes and Stars #1714

Closed

wolmir requested review from maxagin and shcheklein July 11, 2022 20:05

wolmir added the product PR that affects product label Jul 11, 2022

wolmir commented Jul 11, 2022

View reviewed changes

webview/src/shared/components/tooltip/Tooltip.tsx Show resolved Hide resolved

mattseddon reviewed Jul 12, 2022

View reviewed changes

webview/src/experiments/components/table/styles.module.scss Outdated Show resolved Hide resolved

wolmir force-pushed the middle-states-poc branch from 8d7c020 to 4a4e427 Compare July 12, 2022 14:43

codeclimate bot reviewed Jul 12, 2022

View reviewed changes

webview/src/experiments/components/table/Cell.tsx Outdated Show resolved Hide resolved

webview/src/experiments/components/table/Cell.tsx Outdated Show resolved Hide resolved

wolmir force-pushed the middle-states-poc branch 3 times, most recently from 81af148 to c2fd5b1 Compare July 12, 2022 19:49

codeclimate bot reviewed Jul 12, 2022

View reviewed changes

webview/src/experiments/components/table/Cell.tsx Outdated Show resolved Hide resolved

webview/src/experiments/components/table/Cell.tsx Outdated Show resolved Hide resolved

wolmir force-pushed the middle-states-poc branch from 614ff4a to 6f0e69f Compare July 12, 2022 20:47

wolmir requested a review from mattseddon July 12, 2022 22:17

mattseddon reviewed Jul 13, 2022

View reviewed changes

webview/src/experiments/components/App.test.tsx Outdated Show resolved Hide resolved

mattseddon reviewed Jul 13, 2022

View reviewed changes

webview/src/experiments/components/App.test.tsx Outdated Show resolved Hide resolved

mattseddon reviewed Jul 13, 2022

View reviewed changes

webview/src/experiments/components/App.test.tsx Outdated Show resolved Hide resolved

wolmir force-pushed the middle-states-poc branch from f8f8882 to bc9fb31 Compare July 13, 2022 16:40

wolmir requested a review from julieg18 as a code owner July 13, 2022 16:40

wolmir force-pushed the middle-states-poc branch from a6742b3 to 08b2e56 Compare July 14, 2022 14:50

wolmir requested a review from mattseddon July 14, 2022 16:10

wolmir commented Jul 14, 2022

View reviewed changes

mattseddon approved these changes Jul 15, 2022

View reviewed changes

wolmir added 2 commits July 18, 2022 10:21

Add middle states indicator for exp table

079cf50

Add some stars to the Table stories data

Remove styles and coalescing operator

84d777c

wolmir force-pushed the middle-states-poc branch from 08b2e56 to 84d777c Compare July 18, 2022 13:42

wolmir merged commit 53f21f3 into main Jul 18, 2022

wolmir deleted the middle-states-poc branch July 18, 2022 14:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show middle state badges for running, starred and checked sub-rows #2004

Show middle state badges for running, starred and checked sub-rows #2004

wolmir commented Jul 8, 2022 •

edited

Loading

mattseddon commented Jul 11, 2022

wolmir commented Jul 11, 2022

mattseddon Jul 12, 2022

mattseddon Jul 12, 2022

wolmir Jul 12, 2022

wolmir commented Jul 12, 2022

wolmir commented Jul 12, 2022

wolmir Jul 14, 2022

wolmir Jul 14, 2022

wolmir Jul 14, 2022

mattseddon Jul 15, 2022

wolmir Jul 14, 2022

wolmir Jul 14, 2022

mattseddon Jul 15, 2022

mattseddon left a comment

mattseddon Jul 15, 2022

mattseddon Jul 15, 2022

mattseddon Jul 15, 2022

mattseddon Jul 15, 2022

mattseddon Jul 15, 2022

mattseddon Jul 15, 2022

codeclimate bot commented Jul 18, 2022

Show middle state badges for running, starred and checked sub-rows #2004

Show middle state badges for running, starred and checked sub-rows #2004

Conversation

wolmir commented Jul 8, 2022 • edited Loading

mattseddon commented Jul 11, 2022

wolmir commented Jul 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wolmir commented Jul 12, 2022

wolmir commented Jul 12, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

codeclimate bot commented Jul 18, 2022

wolmir commented Jul 8, 2022 •

edited

Loading