Flatten table on sort #4685

julieg18 · 2023-09-18T23:37:05Z

1/2 `main` <= this <= #4691

Demo

Screen.Recording.2023-09-19.at.10.50.32.AM.mov

To Do

Remove possible duplicate rows (Flatten table on sort #4685 (comment)) (done in Collapse duplicate rows when table is flattened #4735)

Fixes #4620

extension/src/experiments/model/index.ts

codeclimate · 2023-09-22T22:34:11Z

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

+import { Experiment } from 'dvc/src/experiments/webview/contract'
+import { TableBody } from './TableBody'
+
+interface SortedTableContentProps {


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

julieg18 · 2023-09-26T14:07:34Z

extension/src/test/fixtures/expShow/sorted/rows.ts

@@ -0,0 +1,348 @@
+import { join } from '../../../util/path'


The large numbers are mainly due to:

adding a new code fixture for Storybook (fixtures/expShow/sorted)

Moving the getRowModel mock function value outside of a describe block inside of TableContent.test.tsx

Probably should have moved the storybook changes in a separate chain pr 😅

extension/src/experiments/model/index.ts

julieg18 · 2023-09-26T14:11:51Z

extension/src/test/fixtures/expShow/sorted/tableData.ts

+import { TableData } from '../../../../experiments/webview/contract'
+
+const data: TableData = {
+  ...defaultData,


sorted/tableData is a slightly simplified (no deps and no selected exps) version of base/tableData. We could simplify further though if preferred.

julieg18 · 2023-09-26T14:17:31Z

webview/src/stories/Table.stories.tsx

@@ -61,10 +62,7 @@ const tableData = getTableState({
  selectedBranches: [],
  selectedForPlotsCount: 2,
  showOnlyChanged: false,
-  sorts: [


Sorts used to be a default addition to the storybooks but since tableData rows are different when there are sorted, I removed the default sorts array (sorts are still shown in our new sorted fixture and in the deeplyNested fixture).

mattseddon · 2023-09-27T04:54:50Z

extension/src/experiments/model/index.ts

+      this.applyFiltersToFlattenedCommits()
+    const rows = []
+
+    for (const { branch, sha } of this.rowOrder) {


[C] Once the table is flattened we could have duplicate entries right next to each other. We will need a way to collapse these rows:

The only thing that will differ between records will be the branch/tags. I think we should consider concatenating the information on multiple lines into a single cell.

Makes sense! I will open a followup issue.

This needs to be fixed before moving on.

This needs to be fixed before moving on.

Oh, ok! Apologies, I misunderstood your comment.

Fixed in #4735

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

mattseddon

Good work.

One item for follow up.

webview/src/test/mockRowModel.ts

julieg18 · 2023-09-27T16:36:25Z

@tapadipti, @dberenbaum we might want to consider flattening the table for sorting in Studio:

Screen.Recording.2023-09-26.at.12.12.39.PM.mov

webview/src/experiments/components/table/content/CommitCellContent.tsx

webview/src/test/mockRowModel.ts

julieg18 · 2023-09-28T12:01:39Z

@julieg18 Will vs code have different ways to sort or will this be the default and only way to sort the table?

Currently flattening the rows is the only way to sort the table, but that could change depending on user feedback.

mattseddon · 2023-09-28T20:24:41Z

In the flattened table, some of the existing functionality may become a bit confusing. Eg, when a parent commit is hidden, all its exps get thidden. This will not be easy to visualize in a flat table.

The extension uses different filtering logic in that it will keep a parent commit in the table if any of its children are unfiltered. We do however change the filtering logic when the table is flat. Each record is filtered individually.

We recently included experiments in sorting, and atm it applies it hierarchically. That is, exps within a commit are sorted (similar to how it is in vs code before this pr?). We have a https://github.com/iterative/studio/issues/7639 to improve this, and we could include flattening the table as one of the options. But I think we should limit some functions in this mode.

That is also how the extension sorts experiments. All the user feedback that we have gotten is that this does not really help.

codeclimate · 2023-09-29T17:06:50Z

webview/src/experiments/components/table/content/BranchCellContent.tsx

+    }
+  } = cell as unknown as CellContext<Experiment, CellValue>
+
+  if (!branch) {


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

codeclimate · 2023-09-29T17:06:51Z

webview/src/experiments/components/table/content/BranchCellContent.tsx

+    )
+  }
+
+  return (


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

codeclimate · 2023-09-29T17:06:51Z

webview/src/experiments/components/table/content/CommitCellContent.tsx

+    )
+  }
+
+  return (


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

codeclimate · 2023-09-29T17:06:51Z

webview/src/experiments/components/table/body/columns/Columns.tsx

+        minSize: 115,
+        size: 115
+      }),
+      columnHelper.accessor(() => COMMIT_COLUMN_ID, {


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

codeclimate · 2023-09-29T17:06:51Z

webview/src/experiments/components/table/body/columns/Columns.tsx

+
+  if (flattenTable) {
+    columns.push(
+      columnHelper.accessor(() => BRANCH_COLUMN_ID, {


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

julieg18

Alright, going to merge this! Last things left to do are:

Remove possible duplicate rows (Flatten table on sort #4685 (comment)) (done in Collapse duplicate rows when table is flattened #4735)
Make "Parent" and "Branch/Tags" columns sticky (Add "Parents" Columns #4691 (comment))

I'll open a followup issue and start looking into the second task this sprint :)

codeclimate · 2023-09-29T22:23:53Z

webview/src/experiments/components/table/body/columns/Columns.tsx

-    minSize: 230,
-    size: 240
-  })
+const getDefaultColumns = (flattenTable: boolean) => {


Function getDefaultColumns has 47 lines of code (exceeds 40 allowed). Consider refactoring.

codeclimate · 2023-09-29T22:23:55Z

Code Climate has analyzed commit abf9707 and detected 11 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1
Duplication	10

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

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

View more on Code Climate.

wip

e10b376

julieg18 added the product PR that affects product label Sep 18, 2023

julieg18 self-assigned this Sep 18, 2023

bit of cleanup

571524c

julieg18 mentioned this pull request Sep 19, 2023

Add "Parents" Columns #4691

Merged

Fix tests for backend

0ed93ea

mattseddon reviewed Sep 21, 2023

View reviewed changes

extension/src/experiments/model/index.ts Show resolved Hide resolved

julieg18 added 2 commits September 22, 2023 09:11

Add a frontend test

d2eeecf

Fix filter bug and add additional tests

1949c5c

codeclimate bot reviewed Sep 22, 2023

View reviewed changes

Fix storybooks and add new fixture

6725b5e

julieg18 commented Sep 26, 2023

View reviewed changes

julieg18 marked this pull request as ready for review September 26, 2023 14:37

julieg18 requested a review from sroy3 as a code owner September 26, 2023 14:37

Fix typo

41c02b4

julieg18 mentioned this pull request Sep 26, 2023

Fix broken "With Multiple Branches" table storybook #4722

Merged

julieg18 requested a review from mattseddon September 26, 2023 15:01

Merge branch 'main' into flatten-table-on-sort

0ecb32b

mattseddon reviewed Sep 27, 2023

View reviewed changes

webview/src/experiments/components/table/body/TableContent.test.tsx Outdated Show resolved Hide resolved

mattseddon approved these changes Sep 27, 2023

View reviewed changes

move mock row model into separate file

bc00ad8

codeclimate bot reviewed Sep 27, 2023

View reviewed changes

julieg18 added 2 commits September 27, 2023 11:23

Add "Parents" Columns (#4691)

d2ded95

Merge branch 'main' into flatten-table-on-sort

9d1210d

codeclimate bot reviewed Sep 27, 2023

View reviewed changes

julieg18 mentioned this pull request Sep 28, 2023

Collapse duplicate rows when table is flattened #4735

Merged

julieg18 added 2 commits September 29, 2023 11:47

Update .codeclimate.json

aac8b50

Merge branch 'main' into flatten-table-on-sort

941f541

codeclimate bot reviewed Sep 29, 2023

View reviewed changes

julieg18 commented Sep 29, 2023

View reviewed changes

Merge branch 'main' into flatten-table-on-sort

abf9707

julieg18 enabled auto-merge (squash) September 29, 2023 22:14

codeclimate bot reviewed Sep 29, 2023

View reviewed changes

julieg18 merged commit 9e7ea16 into main Sep 29, 2023
3 checks passed

julieg18 deleted the flatten-table-on-sort branch September 29, 2023 22:34

julieg18 mentioned this pull request Oct 2, 2023

Improve Table Flattening on Sort #4746

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flatten table on sort #4685

Flatten table on sort #4685

julieg18 commented Sep 18, 2023 •

edited

Loading

codeclimate bot Sep 22, 2023

julieg18 Sep 26, 2023 •

edited

Loading

julieg18 Sep 26, 2023

julieg18 Sep 26, 2023

julieg18 Sep 26, 2023 •

edited

Loading

mattseddon Sep 27, 2023

mattseddon Sep 27, 2023

julieg18 Sep 27, 2023

mattseddon Sep 27, 2023

julieg18 Sep 27, 2023

julieg18 Sep 29, 2023

mattseddon left a comment

julieg18 commented Sep 27, 2023

julieg18 commented Sep 28, 2023

mattseddon commented Sep 28, 2023

codeclimate bot Sep 29, 2023

codeclimate bot Sep 29, 2023

codeclimate bot Sep 29, 2023

codeclimate bot Sep 29, 2023

codeclimate bot Sep 29, 2023

julieg18 left a comment

codeclimate bot Sep 29, 2023

codeclimate bot commented Sep 29, 2023

Flatten table on sort #4685

Flatten table on sort #4685

Conversation

julieg18 commented Sep 18, 2023 • edited Loading

1/2 main <= this <= #4691

Demo

To Do

codeclimate bot Sep 22, 2023

Choose a reason for hiding this comment

julieg18 Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julieg18 Sep 26, 2023 • 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

mattseddon left a comment

Choose a reason for hiding this comment

julieg18 commented Sep 27, 2023

julieg18 commented Sep 28, 2023

mattseddon commented Sep 28, 2023

codeclimate bot Sep 29, 2023

Choose a reason for hiding this comment

codeclimate bot Sep 29, 2023

Choose a reason for hiding this comment

codeclimate bot Sep 29, 2023

Choose a reason for hiding this comment

codeclimate bot Sep 29, 2023

Choose a reason for hiding this comment

codeclimate bot Sep 29, 2023

Choose a reason for hiding this comment

julieg18 left a comment

Choose a reason for hiding this comment

codeclimate bot Sep 29, 2023

Choose a reason for hiding this comment

codeclimate bot commented Sep 29, 2023

julieg18 commented Sep 18, 2023 •

edited

Loading

1/2 `main` <= this <= #4691

julieg18 Sep 26, 2023 •

edited

Loading

julieg18 Sep 26, 2023 •

edited

Loading