Add multiple branches support inside the experiments table #3735

sroy3 · 2023-04-21T14:09:15Z

Part of #1966

When there will be multiple branches, it will look like:

Screen.Recording.2023-04-21.at.9.18.31.AM.mov

Might still need to figure out where to move the "Select branches to show" (not shown here, but will look as before) and the "Switch to all branches view"

If there is only one branch (the current branch and the current behaviour, it will look like that (same as before:

qlty-cloud-legacy · 2023-04-21T14:31:58Z

...ew/src/experiments/components/table/body/commitsAndBranches/CommitsAndBranchesNavigation.tsx

+import tableStyles from '../../styles.module.scss'
+import { ExperimentsState } from '../../../../store'
+
+export const CommitsAndBranchesNavigation: React.FC = () => {


Function CommitsAndBranchesNavigation has 43 lines of code (exceeds 40 allowed). Consider refactoring.

qlty-cloud-legacy · 2023-04-21T14:31:58Z

webview/src/experiments/components/table/body/TableBody.tsx

+  showPreviousRow?: boolean
+}
+
+export const TableBody: React.FC<TableBodyProps> = ({


Similar blocks of code found in 3 locations. Consider refactoring.

mattseddon

Looks good. Left some comments. I do think we will need to provide a way for users to display selected rows from the table closer to each other. Also, need to consider how delta mode fits in with this as well.

I don't think it would hurt to add the Git-merge icon in with the other indicators (Plot, filter, sort) and have the action fire up "Select Branches" as well. The badge could relate to the number of branches selected. The idea is that it will be complementary to the text at the bottom of each branch, not replace it.

🙏🏻

mattseddon · 2023-04-23T23:20:58Z

webview/src/experiments/components/table/body/TableContent.test.tsx

+        }
+      ]
+    })
+  } as unknown as Table<Experiment>


[I] When we add the extension part of this it would be good to add more commits/at least one extra branch to the base test fixture.

[Q] Would also mean we would get the design added to a Story, right?

Yes, as soon as there is more than one branch, the new design appears. I think it might be good to add a story for it right away, so that it's possible to see it without faking it.

mattseddon · 2023-04-23T23:27:21Z

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

+@import '../../../../../shared/variables.scss';
+
+.branchName {
+  padding: 40px 20px 10px;


[C] The next problem we will have to solve is: "how can a user compare two separate records on different branches". For me, this means we need a mechanism to display them directly above/below one another. Maybe we could allow pinning or we could start with something like "show only selected" as a multi-select option.

That will need some design and planning because it does seem like this use case will be very much needed and there is no simple solution for it.

mattseddon · 2023-04-23T23:28:26Z

...ew/src/experiments/components/table/body/commitsAndBranches/CommitsAndBranchesNavigation.tsx

+                isBranchesView ? switchToCommitsView : switchToBranchesView
+              }
+            >
+              {isBranchesView


[Q] Do we need to/should we keep this around long term?

I think people in general feel like this won't be necessary anymore, I don't mind removing it once the whole feature is in. That being said, I personally feel like this could be useful before selecting branches to add as it gives the user an overview of what is in each branch. Also if you do not particularly case about previous commits and have too many branches to easily select, this is a quick way to view everything at once.

This isn't too complicated to remove, nor it is to keep it. The most problematic part about keeping it is where to show it now that we have controls on every branch. I have no preferences about it.

Trying to limit/fight complexity.

mattseddon · 2023-04-23T23:34:22Z

webview/src/experiments/components/Experiments.tsx

    columnResizeMode: 'onChange',
    columns: columns as ColumnDef<Commit, unknown>[],
-    data,
+    // Setting any branch for now just so that it isn't undefined. It does not matter the label for now as it is not shown


[Q] Can we track that this needs to be removed/updated somewhere?

webview/src/experiments/components/table/body/TableContent.tsx

julieg18 · 2023-04-24T14:39:54Z

webview/src/experiments/components/table/body/branchDivider/BranchDivider.tsx

+import { GitMerge } from '../../../../../shared/components/icons'
+
+export const BranchDivider: React.FC<PropsWithChildren> = ({ children }) => (
+  <tbody data-testid="branch-name">


Do we want our branch divider to be a separate body? Would it make more sense, accessibility-wise, for this to be table header row inside its corresponding table body?

julieg18 · 2023-04-24T14:59:57Z

...ew/src/experiments/components/table/body/commitsAndBranches/CommitsAndBranchesNavigation.tsx

+
+  return (
+    <tbody>
+      <tr>


From my knowledge of table accessibility, I think cells inside the table should correspond with the table head if they're not a header. But due to how our table works now with multiple branches I don't see another way to easily show actions below each branch group so probably just have to leave it as is 🤔

julieg18 · 2023-04-24T15:10:33Z

webview/src/experiments/components/table/body/PreviousCommitsRow.tsx

+  isBranchesView,
+  nbColumns
+}) => (
+  <tbody>


Same question for here as with the git branch row about this being in its own separate body.

Just realized that this row is currently a tbody in main 🤔 Must have changed from a header row at some point :)

Changed both for <thead>.

Apologies, I should have been more clear. A table actually can't have multiple theads, I was referring to th cells inside of a tr, whether that's directly inside of a tbody or inside of table. Our rows are made up of multiple tbodys so probably just being inside a direct child of the table would work best.

This is how accessibility standards recommend you add subheaders to tables.

julieg18 · 2023-04-24T15:20:03Z

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

 .errorText {
  color: $error-color;
 }



The table bottom border is showing on the table:

Though in my opinion, this removed border is minor and we can just keep the border on all rows if it's hard to implement. If we decide this, we'll need to remove the corresponding CSS though.

julieg18 · 2023-04-24T15:22:23Z

webview/src/experiments/components/table/body/TableBody.tsx

Is there a way to view how the different branch tables look on my end? Looking at the code, I'm not seeing a way to do this, but I might be missing something :)

From your demo, it looks like the experiment column shadow might be missing but I can't confirm this since, as mentioned, I'm not sure how to test how things look.

Currently adding a story to see it. Otherwise, you have to manually add branches to the rows (we can duplicate the non workspace rows we send to updateRows in App.tsx)

…e/vscode-dvc into multiple-branches-table

qlty-cloud-legacy · 2023-04-25T13:00:54Z

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

        <Icon width={16} height={16} icon={Filter} />
      </Indicator>
+      {featureFlag.ADD_REMOVE_BRANCHES && (
+        <Indicator


Similar blocks of code found in 2 locations. Consider refactoring.

qlty-cloud-legacy · 2023-04-25T14:49:05Z

webview/src/experiments/components/table/body/TableBody.tsx

+  isLast?: boolean
+}
+
+export const TableBody: React.FC<TableBodyProps> = ({


Similar blocks of code found in 2 locations. Consider refactoring.

qlty-cloud-legacy · 2023-04-25T14:49:08Z

Code Climate has analyzed commit 2161709 and detected 4 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1
Duplication	3

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 94.7% (0.1% change).

View more on Code Climate.

Add multiple branches support inside the experiments table

e45414a

sroy3 added the product PR that affects product label Apr 21, 2023

sroy3 self-assigned this Apr 21, 2023

Merge branch 'main' into multiple-branches-table

11ca634

qlty-cloud-legacy bot reviewed Apr 21, 2023

View reviewed changes

Stephanie Roy and others added 3 commits April 21, 2023 16:01

Add tests

f8fefdd

Fix revert

1974c1e

Merge branch 'main' into multiple-branches-table

e89bb63

sroy3 marked this pull request as ready for review April 21, 2023 20:34

sroy3 requested review from julieg18 and mattseddon as code owners April 21, 2023 20:34

Merge branch 'main' into multiple-branches-table

b4a3b68

mattseddon approved these changes Apr 23, 2023

View reviewed changes

julieg18 approved these changes Apr 24, 2023

View reviewed changes

Stephanie Roy added 3 commits April 25, 2023 08:48

Add story + add merge icon in indicators

3b472ca

Merge branch 'main' into multiple-branches-table

28c3d2b

Merge branch 'multiple-branches-table' of https://github.com/iterativ…

7b533f9

…e/vscode-dvc into multiple-branches-table

qlty-cloud-legacy bot reviewed Apr 25, 2023

View reviewed changes

Stephanie Roy added 2 commits April 25, 2023 09:21

Fix e2e tests

cb6589c

Add back last border + changeed tbodys for theads

2161709

qlty-cloud-legacy bot reviewed Apr 25, 2023

View reviewed changes

sroy3 merged commit d0bd6e3 into main Apr 25, 2023

sroy3 deleted the multiple-branches-table branch April 25, 2023 15:35

Add multiple branches support inside the experiments table #3735

Add multiple branches support inside the experiments table #3735

Uh oh!

Conversation

sroy3 commented Apr 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qlty-cloud-legacy bot Apr 21, 2023

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot Apr 21, 2023

Choose a reason for hiding this comment

Uh oh!

mattseddon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julieg18 Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot Apr 25, 2023

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot Apr 25, 2023

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot commented Apr 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sroy3 commented Apr 21, 2023 •

edited

Loading

julieg18 Apr 24, 2023 •

edited

Loading