Change experiments table circle to radio button #2553

mattseddon · 2022-10-10T02:00:59Z

1/2 `main` <- this <- #2567

Related to #2373

This PR updates the experiments table and replaces the existing circles with a radio button. In the follow-up PR I have tried to improve the tooltip displayed when hovering over the radio button. Although not perfect I think this change is an improvement and good enough to ship.

Demo

Screen.Recording.2022-10-12.at.12.51.46.pm.mov

Previous attempts:

Demo

Screen.Recording.2022-10-10.at.12.59.55.pm.mov

With the sub-row indicator moved

Screen.Recording.2022-10-10.at.4.48.45.pm.mov

Left in draft and asked for reviews because I would like everyone to take a look at the idea.

I think we should move the eye + circle to the LHS with the other row action indicators and put any required whitespace between the circle and chevron. Like this:

Demo of the above

Screen.Recording.2022-10-10.at.6.52.23.pm.mov

maxagin · 2022-10-10T02:38:55Z

What is going to be displayed in the exps sidebar section?

mattseddon · 2022-10-10T02:43:38Z

What is going to be displayed in the exps sidebar section?

No change

julieg18 · 2022-10-10T17:18:41Z

Adding the eye definitely makes things more clear for me!

I think we should move the eye + circle to the LHS with the other row action indicators and put any required whitespace between the circle and chevron.

➕ on this idea!

sroy3 · 2022-10-11T13:41:46Z

I think this make it more obvious that there is an action to be done or, but I don't think this would solve the problem by itself.

I don't see anymore relation between the eye and the plots than between the circle and plots.
The eye + circle feels like duplicated information
It starting to be a little crowded on the row

Maybe if there was a new column header that says "Plot/Unplot" and make the bullets a clear column? One downside I'm seeing from this is that the other actions won't be labeled in the same way (star, select).

sroy3

Now that we are having more actions to the left, would it make sense to move the label of the column to something like this?

Or even move the name to the right?

(this would need to make that column be able to resize smaller than it possible at the moment)

sroy3 · 2022-10-12T12:53:51Z

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

+          testId={'row-action-plot'}
+          tooltipContent={getTooltipContent(!!bulletColor, 'Plot')}
+        >
+          <span


This is fine for now, but we definitely could change that for a button like we said. We could then have toggleExperiment on click only and accessibility would be built in the component. There must be some default styles to the button that need to be overwritten, but other than that it should be a simple change.

sroy3 · 2022-10-12T12:55:22Z

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

+
+  align-items: center;
+  justify-content: center;
+  align-self: center;


Is this still needed? I vaguely remember us adding the property and it not doing anything.

sroy3 · 2022-10-12T12:56:03Z

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

+  border: calc(var(--border-width) * 1px) solid $checkbox-border;
+  box-sizing: border-box;
+  cursor: pointer;
+  height: calc(var(--design-unit) * 4px);


We could make a variable out of that and use it for height, width and border-radius

pulled out variable

sroy3 · 2022-10-12T12:59:25Z

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

+  box-sizing: border-box;
+  cursor: pointer;
+  height: calc(var(--design-unit) * 4px);
+  outline: none;


This is useful if we transform the span to a button. Otherwise, it doesn't do anything. For transforming the element to a <button />, the only other styles needed is padding: 0 and border: none I think. We'll be able to remove box-sizing: border-box too as its already the default on a button.

changed to a button

julieg18

Great work, Matt!

Now that we are having more actions to the left, would it make sense to move the label of the column to something like this?

Or even move the name to the right?

(this would need to make that column be able to resize smaller than it possible at the moment)

Personally, I find the Experiments label being in the "center" to look like of buggy. As for the second option, I believe it is recommended that we keep text left-aligned instead of right-aligned for readability. No strong opinion on this though :)

julieg18 · 2022-10-12T16:19:40Z

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

@@ -51,11 +59,17 @@ export const CellRowAction: React.FC<CellRowActionProps> = ({
  )
 }

+const getTooltipContent = (determiner: boolean, text: string): string =>


Great idea for creating the text!

julieg18 · 2022-10-12T16:29:30Z

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

+          testId={'row-action-plot'}
+          tooltipContent={getTooltipContent(!!bulletColor, 'Plot')}
+        >
+          <span


shcheklein · 2022-10-13T20:07:23Z

One caveat (not critical for now) is checkpoints (and resume / continue logic) when line breaks between the checkpoints. In this case line gives a hint.

LGTM otherwise, let's proceed with this. I would say though that I don't think this is enough. I think we need more explicit things, buttons, etc to connect both webviews.

mattseddon · 2022-10-14T06:03:06Z

Now that we are having more actions to the left, would it make sense to move the label of the column to something like this?

Or even move the name to the right? (this would need to make that column be able to resize smaller than it possible at the moment)

@sroy3 this is good. I managed to move "Experiment" to the right ~~but failed with the experiment names. Can you help me out with the code?~~

Edit: I have "fixed" the problem but in order to do so I've changed the behaviour of the row

Screen.Recording.2022-10-14.at.7.16.15.pm.mov

Screen.Recording.2022-10-14.at.7.20.10.pm.mov

This is what things look like spreading the line from the opposite direction:

Screen.Recording.2022-10-14.at.7.21.44.pm.mov

The last idea that I had is to always keep the display name on the second line as per:

Screen.Recording.2022-10-14.at.8.33.20.pm.mov

The last one is probably my preferred option.

WDYT? Am I overthinking this?

mattseddon · 2022-10-14T09:55:38Z

@julieg18 @sroy3 for moving experiment names to the right of the first cell please take a look at the storybook and last commit. LMK what you think, thanks.

sroy3 · 2022-10-14T13:09:29Z

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

+          <button
+            className={styles.bullet}
+            style={{ color: bulletColor }}
+            {...clickAndEnterProps(toggleExperiment)}


We can change this to a basic onClick event since all keyboard event are already handled by the button.

This ended up being a button inside a button so I had to move the action into <Indicator/> and changed it back to a <span/>

I will make the same change for the other row actions in a follow-up (the change is bigger than I anticipated).

sroy3 · 2022-10-14T13:13:30Z

The last one is probably my preferred option.

WDYT? Am I overthinking this?

I think I'm with you on that one. The last option looks good. The first one is ok too and I wouldn't mind if it was what we finally went with.

julieg18 · 2022-10-14T14:17:59Z

The last one is probably my preferred option.
WDYT? Am I overthinking this?

Last one is also my preferred option! The first one shows what appears to be secondary text (gray and small) before the primary text (white and large) and the second one looks kind of buggy to me with how the text jumps.

codeclimate · 2022-10-15T01:10:23Z

Code Climate has analyzed commit 92be048 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.8% (0.0% change).

View more on Code Climate.

shcheklein · 2022-10-15T01:32:51Z

Q: how will to look like with three commits? (my concern that we've completely flattened out the table - commit name, experiments (and checkpoints?) are all on the same level now? If that is the case, then we are loosing important information - hierarchy.

mattseddon · 2022-10-15T01:36:02Z

Q: how will to look like with three commits? (my concern that we've completely flattened out the table - commit name, experiments (and checkpoints?) are all on the same level now? If that is the case, then we are loosing important information - hierarchy.

shcheklein · 2022-10-15T01:38:20Z

Hmm, I prefer it way better the way it was before. It was more informative (in an important way - there is child-parent relationship). We've removed lines and hierarchy. It means either we'll have to implement something like in studio (sections per branch) or we need to go back and introduce the hierarchy back.

mattseddon · 2022-10-15T01:41:44Z

Hmm, I prefer it way better the way it was before.

Which part of the change?

shcheklein · 2022-10-15T01:53:02Z

Sorry, I meant the names. Buttons are fine and definitely an improvement!

mattseddon · 2022-10-15T01:56:05Z

Sorry, I meant the names. Buttons are fine and definitely an improvement!

Ok, I will raise a PR to revert.

add eye to experiments table to demonstrate plotting

2ec9bcb

mattseddon added the product PR that affects product label Oct 10, 2022

mattseddon self-assigned this Oct 10, 2022

mattseddon requested review from julieg18, sroy3, maxagin, shcheklein and wolmir October 10, 2022 02:03

mattseddon added 4 commits October 10, 2022 16:54

move indicator onto eye

08d4b3c

move indicators to right of chevron

5851cf8

move chevrons left

1f79ede

bring back indicator for workspace row

a372695

mattseddon force-pushed the add-eye branch from bc7881c to a372695 Compare October 10, 2022 07:51

mattseddon added 5 commits October 12, 2022 12:08

switch eye for radio button and improve tooltip

ad47e12

switch ternary to be in the correct direction

043bb2b

improve tooltip

195e1fd

fix spinner

34c3e01

destructure row action props to get toggle experiment

49e625b

mattseddon changed the title ~~Add eye to experiments table to connect to plots view~~ Change experiments table circle to radio button Oct 12, 2022

revert tooltip in order to split PR

fec91da

mattseddon force-pushed the add-eye branch from 3a3aa6d to e55f7d7 Compare October 12, 2022 01:40

mattseddon added 2 commits October 12, 2022 12:43

Merge branch 'main' into add-eye

447d353

resort props

9c0e81d

mattseddon force-pushed the add-eye branch from e55f7d7 to 9c0e81d Compare October 12, 2022 01:46

mattseddon marked this pull request as ready for review October 12, 2022 01:59

mattseddon mentioned this pull request Oct 12, 2022

Add hints into experiment table row action tooltips #2567

Merged

sroy3 approved these changes Oct 12, 2022

View reviewed changes

julieg18 approved these changes Oct 12, 2022

View reviewed changes

address review feedback

07e317f

mattseddon added 2 commits October 14, 2022 20:53

move experiment names to the right

4c0a5b6

Merge branch 'main' into add-eye

4ad90b7

sroy3 reviewed Oct 14, 2022

View reviewed changes

rework radio button to be single button

d748b48

shcheklein mentioned this pull request Oct 14, 2022

Update the DVC: Get Started / Plots Dashboard #2573

Merged

Merge branch 'main' into add-eye

92be048

mattseddon enabled auto-merge (squash) October 15, 2022 01:11

mattseddon disabled auto-merge October 15, 2022 01:11

mattseddon merged commit 94f2497 into main Oct 15, 2022

mattseddon deleted the add-eye branch October 15, 2022 01:29

mattseddon mentioned this pull request Oct 15, 2022

Revert moving experiment names to right in table's experiment column (maintain heirarchy) #2594

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change experiments table circle to radio button #2553

Change experiments table circle to radio button #2553

mattseddon commented Oct 10, 2022 •

edited

Loading

maxagin commented Oct 10, 2022

mattseddon commented Oct 10, 2022

julieg18 commented Oct 10, 2022

sroy3 commented Oct 11, 2022 •

edited

Loading

sroy3 left a comment •

edited

Loading

sroy3 Oct 12, 2022

julieg18 Oct 12, 2022

sroy3 Oct 12, 2022

mattseddon Oct 14, 2022

sroy3 Oct 12, 2022

mattseddon Oct 14, 2022

sroy3 Oct 12, 2022

mattseddon Oct 14, 2022

julieg18 left a comment

julieg18 Oct 12, 2022

julieg18 Oct 12, 2022

shcheklein commented Oct 13, 2022

mattseddon commented Oct 14, 2022 •

edited

Loading

mattseddon commented Oct 14, 2022 •

edited

Loading

sroy3 Oct 14, 2022

mattseddon Oct 14, 2022 •

edited

Loading

mattseddon Oct 15, 2022

sroy3 commented Oct 14, 2022

julieg18 commented Oct 14, 2022 •

edited

Loading

codeclimate bot commented Oct 15, 2022

shcheklein commented Oct 15, 2022

mattseddon commented Oct 15, 2022

shcheklein commented Oct 15, 2022

mattseddon commented Oct 15, 2022

shcheklein commented Oct 15, 2022

mattseddon commented Oct 15, 2022

Change experiments table circle to radio button #2553

Change experiments table circle to radio button #2553

Conversation

mattseddon commented Oct 10, 2022 • edited Loading

1/2 main <- this <- #2567

Demo

Demo

With the sub-row indicator moved

Demo of the above

maxagin commented Oct 10, 2022

mattseddon commented Oct 10, 2022

julieg18 commented Oct 10, 2022

sroy3 commented Oct 11, 2022 • edited Loading

sroy3 left a comment • edited Loading

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

julieg18 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shcheklein commented Oct 13, 2022

mattseddon commented Oct 14, 2022 • edited Loading

mattseddon commented Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

mattseddon Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sroy3 commented Oct 14, 2022

julieg18 commented Oct 14, 2022 • edited Loading

codeclimate bot commented Oct 15, 2022

shcheklein commented Oct 15, 2022

mattseddon commented Oct 15, 2022

shcheklein commented Oct 15, 2022

mattseddon commented Oct 15, 2022

shcheklein commented Oct 15, 2022

mattseddon commented Oct 15, 2022

mattseddon commented Oct 10, 2022 •

edited

Loading

1/2 `main` <- this <- #2567

sroy3 commented Oct 11, 2022 •

edited

Loading

sroy3 left a comment •

edited

Loading

mattseddon commented Oct 14, 2022 •

edited

Loading

mattseddon commented Oct 14, 2022 •

edited

Loading

mattseddon Oct 14, 2022 •

edited

Loading

julieg18 commented Oct 14, 2022 •

edited

Loading