Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions extension/src/experiments/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,7 @@ export class Experiments extends BaseRepository<TableData> {
columnOrder: this.columns.getColumnOrder(),
columnWidths: this.columns.getColumnWidths(),
columns: this.columns.getSelected(),
filters: this.experiments.getFilterPaths(),
hasCheckpoints: this.hasCheckpoints(),
hasColumns: this.columns.hasColumns(),
hasRunningExperiment: this.experiments.hasRunningExperiment(),
Expand Down
4 changes: 4 additions & 0 deletions extension/src/experiments/model/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,10 @@ export class ExperimentsModel extends ModelWithPersistence {
return [...this.filters.values()]
}

public getFilterPaths() {
return this.getFilters().map(({ path }) => path)
}

public canAutoApplyFilters(...filterIdsToRemove: string[]): boolean {
if (!this.useFiltersForSelection) {
return true
Expand Down
1 change: 1 addition & 0 deletions extension/src/experiments/webview/contract.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ export type TableData = {
hasRunningExperiment: boolean
rows: Row[]
sorts: SortDefinition[]
filters: string[]
}

export type InitiallyUndefinedTableData = TableData | undefined
4 changes: 4 additions & 0 deletions extension/src/test/fixtures/expShow/deeplyNested.ts
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,10 @@ const deeplyNestedTableData: TableData = {
changes: [],
columnOrder: [],
columnWidths: {},
filters: [
'params:params.yaml:nested1.doubled',
'params:params.yaml:nested1%2Enested2%2Enested3.nested4.nested5b.doubled'
],
hasCheckpoints: false,
hasRunningExperiment: false,
sorts: [
Expand Down
1 change: 1 addition & 0 deletions extension/src/test/fixtures/expShow/tableData.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import columnsFixture from './columns'
const tableDataFixture: TableData = {
rows: rowsFixture,
columns: columnsFixture,
filters: [],
hasCheckpoints: true,
hasRunningExperiment: true,
hasColumns: true,
Expand Down
2 changes: 2 additions & 0 deletions extension/src/test/suite/experiments/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ suite('Experiments Test Suite', () => {
columnOrder: [],
columnWidths: {},
columns: columnsFixture,
filters: [],
hasCheckpoints: true,
hasColumns: true,
hasRunningExperiment: true,
Expand Down Expand Up @@ -683,6 +684,7 @@ suite('Experiments Test Suite', () => {
columnOrder: [],
columnWidths: {},
columns: [],
filters: [],
hasCheckpoints: true,
hasColumns: true,
hasRunningExperiment: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ suite('Experiments Filter By Tree Test Suite', () => {
columnOrder: [],
columnWidths: {},
columns: columnsFixture,
filters: [accuracyPath],
hasCheckpoints: true,
hasColumns: true,
hasRunningExperiment: true,
Expand Down Expand Up @@ -138,6 +139,7 @@ suite('Experiments Filter By Tree Test Suite', () => {
columnOrder: [],
columnWidths: {},
columns: columnsFixture,
filters: [],
hasCheckpoints: true,
hasColumns: true,
hasRunningExperiment: true,
Expand Down
5 changes: 3 additions & 2 deletions webview/src/experiments/components/Experiments.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ import { GetStarted } from '../../shared/components/getStarted/GetStarted'
import { DragDropProvider } from '../../shared/components/dragDrop/DragDropContext'
import { EmptyState } from '../../shared/components/emptyState/EmptyState'

const DEFAULT_COLUMN_WIDTH = 75
const MINIMUM_COLUMN_WIDTH = 50
const DEFAULT_COLUMN_WIDTH = 90
const MINIMUM_COLUMN_WIDTH = 90
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[I] There are a couple of issues here.

  1. 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
  1. On resize is causing the sort direction to change.
Screen.Recording.2022-05-31.at.8.52.01.am.mov

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this @mattseddon @shcheklein

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wolmir got it, I like it! :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's just for show

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen Shot 2022-06-02 at 20 15 40


const timeFormatter = new Intl.DateTimeFormat([], {
hour: '2-digit',
Expand Down Expand Up @@ -138,6 +138,7 @@ export const ExperimentsTable: React.FC<{
columnOrder: [],
columnWidths: {},
columns: [],
filters: [],
hasCheckpoints: false,
hasColumns: false,
hasRunningExperiment: false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,15 @@ export const MergedHeaderGroups: React.FC<{
headerGroup: HeaderGroup<Experiment>
columns: HeaderGroup<Experiment>[]
sorts: SortDefinition[]
filters: string[]
orderedColumns: Column[]
onDragUpdate: OnDragOver
onDragStart: OnDragStart
onDragEnd: OnDrop
}> = ({
headerGroup,
sorts,
filters,
columns,
orderedColumns,
onDragUpdate,
Expand All @@ -41,6 +43,7 @@ export const MergedHeaderGroups: React.FC<{
column={column}
columns={columns}
sorts={sorts}
filters={filters}
onDragOver={onDragUpdate}
onDragStart={onDragStart}
onDrop={onDragEnd}
Expand Down
50 changes: 0 additions & 50 deletions webview/src/experiments/components/table/SortPicker.tsx

This file was deleted.

84 changes: 31 additions & 53 deletions webview/src/experiments/components/table/Table.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,13 @@
*/
/* eslint jest/expect-expect: ["error", { "assertFunctionNames": ["expect", "expectHeaders"] }] */
import '@testing-library/jest-dom/extend-expect'
import {
cleanup,
fireEvent,
render,
screen,
within
} from '@testing-library/react'
import { cleanup, fireEvent, render, screen } from '@testing-library/react'
import { Experiment, TableData } from 'dvc/src/experiments/webview/contract'
import { MessageFromWebviewType } from 'dvc/src/webview/contract'
import React from 'react'
import { TableInstance } from 'react-table'
import tableDataFixture from 'dvc/src/test/fixtures/expShow/tableData'
import { SortOrderLabel } from './SortPicker'
import { SortOrder } from './TableHeader'
import { Table } from './Table'
import styles from './styles.module.scss'
import { ExperimentsTable } from '../Experiments'
Expand Down Expand Up @@ -112,6 +106,7 @@ describe('Table', () => {
columnOrder: [],
columnWidths: {},
columns: [],
filters: [],
hasCheckpoints: false,
hasColumns: true,
hasRunningExperiment: false,
Expand Down Expand Up @@ -144,54 +139,41 @@ describe('Table', () => {
const mockColumnName = 'C'
const mockColumnPath = 'params:C'

const findSortableColumn = async (headerText = mockColumnName) =>
await screen.findByText(headerText)
const findSortableColumn = async () =>
await screen.findByTestId(`header-${mockColumnPath}`)

const clickOnSortOption = async (optionLabel: SortOrderLabel) => {
const column = await findSortableColumn()
const clickOnSortOption = async (optionLabel: SortOrder) => {
const column = await screen.findByText(mockColumnName)
fireEvent.contextMenu(column, {
bubbles: true
})
const columnMenu = await screen.findAllByRole('menu')

const sortOption = await within(columnMenu[0]).findByText(optionLabel)
const sortOption = await screen.findByText(optionLabel)
fireEvent.click(sortOption)
}

describe('Given no sorting is present yet', () => {
it('should add an ascending sort to the column path when the user clicks on the Ascending option', async () => {
describe('Sortable column', () => {
it('should not not have a sorting indicator if it is not sorted yet', () => {
renderExperimentsTable()
await clickOnSortOption(SortOrderLabel.ASCENDING)
expect(mockedPostMessage).toBeCalledWith({
payload: {
descending: false,
path: mockColumnPath
},
type: MessageFromWebviewType.SORT_COLUMN
})
const sortIcons = screen.queryAllByTestId('sorting-indicator')

expect(sortIcons.length).toBe(0)
})

it('should add a descending sort to the column path when the user clicks on the Descending option', async () => {
it('should be able to add an ascending sort to the column, if it is not sorted yet', async () => {
renderExperimentsTable()
await clickOnSortOption(SortOrderLabel.DESCENDING)
await clickOnSortOption(SortOrder.ASCENDING)

expect(mockedPostMessage).toBeCalledWith({
payload: {
descending: true,
descending: false,
path: mockColumnPath
},
type: MessageFromWebviewType.SORT_COLUMN
})
})

it('should not do anything when the user clicks on the None option', async () => {
renderExperimentsTable()
await clickOnSortOption(SortOrderLabel.NONE)
expect(mockedPostMessage).not.toHaveBeenCalled()
})
})

describe('Given an initial ascending column sort', () => {
it('should add a descending sort to the column path when the user clicks on the Descending option', async () => {
it('should add a descending sort to the column, when clicking on the descending option', async () => {
renderExperimentsTable({
...sortingTableDataFixture,
sorts: [
Expand All @@ -201,7 +183,12 @@ describe('Table', () => {
}
]
})
await clickOnSortOption(SortOrderLabel.DESCENDING)

const column = await findSortableColumn()
expect(column).toHaveClass('sortingHeaderCellAsc')

await clickOnSortOption(SortOrder.DESCENDING)

expect(mockedPostMessage).toBeCalledWith({
payload: {
descending: true,
Expand All @@ -211,31 +198,22 @@ describe('Table', () => {
})
})

it('should not do anything when the user clicks on the Ascending option', async () => {
it('should remove the column sort if the remove option is selected', async () => {
renderExperimentsTable({
...sortingTableDataFixture,
sorts: [
{
descending: false,
descending: true,
path: mockColumnPath
}
]
})
await clickOnSortOption(SortOrderLabel.ASCENDING)
expect(mockedPostMessage).not.toHaveBeenCalled()
})

it('should remove the column sort when the user clicks on the None option', async () => {
renderExperimentsTable({
...sortingTableDataFixture,
sorts: [
{
descending: false,
path: mockColumnPath
}
]
})
await clickOnSortOption(SortOrderLabel.NONE)
const column = await findSortableColumn()
expect(column).toHaveClass('sortingHeaderCellDesc')

await clickOnSortOption(SortOrder.NONE)

expect(mockedPostMessage).toBeCalledWith({
payload: mockColumnPath,
type: MessageFromWebviewType.REMOVE_COLUMN_SORT
Expand Down
17 changes: 14 additions & 3 deletions webview/src/experiments/components/table/Table.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -101,13 +101,24 @@ export const Table: React.FC<TableProps & WithChanges> = ({
tableData
}) => {
const { getTableProps, rows } = instance
const { sorts, columns, changes, hasCheckpoints, hasRunningExperiment } =
tableData
const {
filters,
sorts,
columns,
changes,
hasCheckpoints,
hasRunningExperiment
} = tableData

return (
<div className={styles.tableContainer}>
<div {...getTableProps({ className: styles.table })}>
<TableHead instance={instance} sorts={sorts} columns={columns} />
<TableHead
instance={instance}
sorts={sorts}
filters={filters}
columns={columns}
/>
{rows.map(row => (
<TableBody
row={row}
Expand Down
3 changes: 3 additions & 0 deletions webview/src/experiments/components/table/TableHead.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@ interface TableHeadProps {
instance: TableInstance<Experiment>
columns: Column[]
sorts: SortDefinition[]
filters: string[]
}

export const TableHead: React.FC<TableHeadProps> = ({
filters,
instance: {
headerGroups,
setColumnOrder,
Expand Down Expand Up @@ -90,6 +92,7 @@ export const TableHead: React.FC<TableHeadProps> = ({
headerGroup={headerGroup}
columns={allHeaders}
sorts={sorts}
filters={filters}
onDragStart={onDragStart}
onDragUpdate={onDragUpdate}
onDragEnd={onDragEnd}
Expand Down
Loading