Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Group table styles #3667

Merged
merged 9 commits into from
Apr 19, 2023
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
2 changes: 1 addition & 1 deletion webview/src/experiments/components/AddStage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export const AddStage: React.FC<AddStageProps> = ({ hasValidDvcYaml }) => (
disabled={!hasValidDvcYaml}
/>
{!hasValidDvcYaml && (
<p className={styles.error}>
<p className={styles.errorText}>
Your dvc.yaml file should contain valid yaml before adding any pipeline
stages.
</p>
Expand Down
14 changes: 0 additions & 14 deletions webview/src/experiments/components/App.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -76,20 +76,6 @@ afterEach(() => {
})

describe('App', () => {
describe('Sorting Classes', () => {
it('should apply the sortingHeaderCellDesc class to a header cell of a column sorted descending', () => {
renderTableWithPlaceholder()

const headerCell = screen.getByTestId(
'header-params:params.yaml:nested1.doubled'
)

expect(
headerCell.classList.contains(styles.sortingHeaderCellDesc)
).toBeTruthy()
})
})

it('should send a message to the extension on the first render', () => {
renderTable(undefined, true)
expect(mockPostMessage).toHaveBeenCalledWith({
Expand Down
2 changes: 1 addition & 1 deletion webview/src/experiments/components/Experiments.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ const getDefaultColumnWithIndicatorsPlaceHolder = () =>
}
} = cell as unknown as CellContext<Experiment, CellValue>
return (
<div className={styles.experimentCellContents}>
<div className={styles.experimentCellText}>
<span>{label}</span>
{displayName && (
<CellSecondaryName
Expand Down
14 changes: 7 additions & 7 deletions webview/src/experiments/components/table/Cell.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -55,16 +55,16 @@ export const FirstCell: React.FC<
const { toggleExperiment } = rowActionsProps

return (
<td className={styles.experimentCell}>
<td className={cx(styles.experimentsTd, styles.experimentCell)}>
<div className={styles.innerCell} style={{ width: getSize() }}>
<CellRowActions status={status} {...rowActionsProps} />
<RowExpansionButton row={row} />
{getIsPlaceholder() ? null : (
<ErrorTooltip error={error}>
<div
className={cx(styles.experimentCellContentsContainer, {
[styles.workspaceChange]: changesIfWorkspace,
[styles.error]: error
className={cx(styles.experimentCellTextWrapper, {
[styles.workspaceChangeText]: changesIfWorkspace,
[styles.errorText]: error
})}
{...clickAndEnterProps(
toggleExperiment,
Expand Down Expand Up @@ -92,9 +92,9 @@ export const CellWrapper: React.FC<
> = ({ cell, cellId, changes }) => {
return (
<td
className={cx({
[styles.workspaceChange]: changes?.includes(cell.column.id),
[styles.depChange]: cellHasChanges(cell.getValue() as CellValue)
className={cx(styles.experimentsTd, {
[styles.workspaceChangeText]: changes?.includes(cell.column.id),
[styles.depChangeText]: cellHasChanges(cell.getValue() as CellValue)
})}
data-testid={cellId}
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ export const CellSecondaryName: React.FC<{
}> = ({ displayName, commit, sha }) => {
const children = (
<span className={styles.experimentCellSecondaryName}>
{commit && <Icon width={14} height={14} icon={GitCommit} />} {displayName}
{commit && <Icon width={14} height={14} icon={GitCommit} />}{' '}
<span>{displayName}</span>
</span>
)
if (!commit) {
Expand Down
2 changes: 1 addition & 1 deletion webview/src/experiments/components/table/Row.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ const getRowClassNames = (
) => {
return cx(
className,
styles.tr,
styles.experimentsTr,
styles.bodyRow,
getExperimentTypeClass(original),
cond(
Expand Down
27 changes: 9 additions & 18 deletions webview/src/experiments/components/table/Table.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,6 @@ describe('Table', () => {
const mockColumnName = 'C'
const mockColumnPath = 'params:C'

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

const clickOnSortOption = async (optionLabel: SortOrder) => {
const column = await screen.findByText(mockColumnName)
fireEvent.contextMenu(column, {
Expand Down Expand Up @@ -113,9 +110,6 @@ describe('Table', () => {
]
})

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

await clickOnSortOption(SortOrder.DESCENDING)

expect(mockedPostMessage).toHaveBeenCalledWith({
Expand All @@ -138,9 +132,6 @@ describe('Table', () => {
]
})

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

await clickOnSortOption(SortOrder.NONE)

expect(mockedPostMessage).toHaveBeenCalledWith({
Expand Down Expand Up @@ -174,17 +165,17 @@ describe('Table', () => {

const workspaceCell = await screen.findByText(EXPERIMENT_WORKSPACE_ID)

expect(workspaceCell?.className.includes(styles.workspaceChange)).toBe(
false
)
expect(
workspaceCell?.className.includes(styles.workspaceChangeText)
).toBe(false)
})

it("should have the workspaceChange class on the workspace's first cell (text) if there are workspace changes", () => {
renderExperimentsTable({ changes: ['something_changed'] })

const workspaceCell = screen.getByTestId('id___workspace')

expect(workspaceCell.className.includes(styles.workspaceChange)).toBe(
expect(workspaceCell.className.includes(styles.workspaceChangeText)).toBe(
true
)
})
Expand All @@ -194,23 +185,23 @@ describe('Table', () => {

const row = await screen.findByTestId('Created___workspace')

expect(row?.className.includes(styles.workspaceChange)).toBe(false)
expect(row?.className.includes(styles.workspaceChangeText)).toBe(false)
})

it('should not have the workspaceChange class on a cell if there are changes to other columns but not this one', async () => {
renderExperimentsTable({ changes: ['a_change'] })

const row = await screen.findByTestId('Created___workspace')

expect(row?.className.includes(styles.workspaceChange)).toBe(false)
expect(row?.className.includes(styles.workspaceChangeText)).toBe(false)
})

it('should have the workspaceChange class on a cell if there are changes matching the column id', async () => {
renderExperimentsTable({ changes: ['Created'] })

const row = await screen.findByTestId('Created___workspace')

expect(row?.className.includes(styles.workspaceChange)).toBe(true)
expect(row?.className.includes(styles.workspaceChangeText)).toBe(true)
})
})

Expand Down Expand Up @@ -391,13 +382,13 @@ describe('Table', () => {
dragEnter(startingNode, targetNode.id, DragEnterDirection.AUTO)

expect(
header?.classList.contains(styles.headerCellDropTarget)
header?.classList.contains(styles.dropTargetHeaderCell)
).toBeTruthy()

dragLeave(targetNode)

expect(
header?.classList.contains(styles.headerCellDropTarget)
header?.classList.contains(styles.dropTargetHeaderCell)
).toBeFalsy()
})

Expand Down
5 changes: 4 additions & 1 deletion webview/src/experiments/components/table/Table.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,10 @@ export const Table: React.FC<TableProps> = ({
>
{/* eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions */}
<table
className={cx(expColumnNeedsShadow && styles.withExpColumnShadow)}
className={cx(
styles.experimentsTable,
expColumnNeedsShadow && styles.withExpColumnShadow
)}
ref={tableRef}
onKeyUp={e => {
if (e.key === 'Escape') {
Expand Down
8 changes: 4 additions & 4 deletions webview/src/experiments/components/table/TableBody.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -78,19 +78,19 @@ export const TableBody: React.FC<
<>
{row.index === 2 && row.depth === 0 && (
<tbody>
<tr className={cx(styles.tr, styles.previousCommitsRow)}>
<td className={styles.th}>
<tr className={cx(styles.experimentsTr, styles.previousCommitsRow)}>
<td className={styles.experimentsTd}>
{isBranchesView ? 'Other Branches' : 'Previous Commits'}
</td>
<td
className={styles.th}
className={styles.experimentsTd}
colSpan={row.getAllCells().length - 1}
></td>
</tr>
</tbody>
)}
<tbody
className={cx(styles.rowGroup, styles.tbody, styles.normalRowGroup, {
className={cx(styles.rowGroup, {
[styles.experimentGroup]: row.depth > 0,
[styles.expandedGroup]: row.getIsExpanded() && row.subRows.length > 0
})}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ interface ErrorCellProps {

export const ErrorCell: React.FC<ErrorCellProps> = ({ error }) => (
<ErrorTooltip error={error}>
<div className={cx(styles.innerCell, styles.error)}>
<div className={cx(styles.innerCell, styles.errorText)}>
<CellContents>!</CellContents>
</div>
</ErrorTooltip>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ interface HeaderProps {
export const Header: React.FC<HeaderProps> = ({ name }) => {
return (
<OverflowHoverTooltip content={name}>
<div className={styles.headerCellWrapper}>
<div className={styles.headerCellText}>
<span>{name}</span>
</div>
</OverflowHoverTooltip>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React from 'react'
import cx from 'classnames'
import { Experiment } from 'dvc/src/experiments/webview/contract'
import { HeaderGroup, Header } from '@tanstack/react-table'
import { TableHeader } from './TableHeader'
Expand Down Expand Up @@ -27,7 +28,7 @@ export const MergedHeaderGroups: React.FC<{
onlyOneLine
}) => {
return (
<tr className={styles.headRow}>
<tr className={cx(styles.experimentsTr, styles.headRow)}>
{headerGroup.headers.map((header: Header<Experiment, unknown>) => (
<TableHeader
setExpColumnNeedsShadow={setExpColumnNeedsShadow}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import {
isExperimentColumn
} from '../../../util/columns'
import { DragFunction } from '../../../../shared/components/dragDrop/Draggable'
import styles from '../styles.module.scss'

interface TableHeadProps {
instance: Table<Experiment>
root: HTMLElement | null
Expand Down Expand Up @@ -117,7 +119,7 @@ export const TableHead = ({
}

return (
<thead ref={wrapper}>
<thead className={styles.experimentsThead} ref={wrapper}>
{headerGroups.map(headerGroup => (
<MergedHeaderGroups
key={headerGroup.id}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,7 @@ import { Header } from '@tanstack/react-table'
import cx from 'classnames'
import { useInView } from 'react-intersection-observer'
import { TableHeaderCellContents } from './TableHeaderCellContents'
import {
ContextMenuContent,
getMenuOptions,
SortOrder
} from './ContextMenuContent'
import { ContextMenuContent, getMenuOptions } from './ContextMenuContent'
import styles from '../styles.module.scss'
import { isExperimentColumn, isFirstLevelHeader } from '../../../util/columns'
import { ExperimentsState } from '../../../store'
Expand Down Expand Up @@ -39,28 +35,22 @@ const calcResizerHeight = (header: Header<Experiment, unknown>) => {
const getHeaderPropsArgs = (
header: Header<Experiment, unknown>,
headerDropTargetId: string,
sortEnabled: boolean,
sortOrder: SortOrder,
onlyOneLine?: boolean
) => {
const columnWithGroup = header.column.columnDef as ColumnWithGroup
return {
className: cx(
styles.experimentsTh,
header.isPlaceholder ? styles.placeholderHeaderCell : styles.headerCell,
{
[styles.paramHeaderCell]: columnWithGroup.group === ColumnType.PARAMS,
[styles.metricHeaderCell]: columnWithGroup.group === ColumnType.METRICS,
[styles.depHeaderCell]: columnWithGroup.group === ColumnType.DEPS,
[styles.createdHeaderCell]: header.id === 'Created',
[styles.firstLevelHeader]: isFirstLevelHeader(header.column.id),
[styles.leafHeader]: header.subHeaders === undefined,
[styles.menuEnabled]: sortEnabled,
[styles.sortingHeaderCellAsc]: sortOrder === SortOrder.ASCENDING,
[styles.sortingHeaderCellDesc]:
sortOrder === SortOrder.DESCENDING && !header.isPlaceholder,
[styles.oneRowHeaderCell]: onlyOneLine
},
headerDropTargetId === header.id && styles.headerCellDropTarget
[styles.firstLevelHeaderCell]: isFirstLevelHeader(header.column.id),
[styles.leafHeaderCell]: header.subHeaders === undefined,
[styles.oneRowHeaderCell]: onlyOneLine,
[styles.dropTargetHeaderCell]: headerDropTargetId === header.id
}
),
style: {
position: undefined
Expand Down Expand Up @@ -161,13 +151,7 @@ export const TableHeaderCell: React.FC<{
trigger="contextmenu"
>
<th
{...getHeaderPropsArgs(
header,
headerDropTargetId,
isSortable,
sortOrder,
onlyOneLine
)}
{...getHeaderPropsArgs(header, headerDropTargetId, onlyOneLine)}
data-testid={`header-${id}${previousPlaceholder}`}
key={id}
tabIndex={0}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React, { useEffect } from 'react'
import cx from 'classnames'
import { ColumnType, Experiment } from 'dvc/src/experiments/webview/contract'
import { Experiment } from 'dvc/src/experiments/webview/contract'
import { flexRender, Header } from '@tanstack/react-table'
import { SortOrder } from './ContextMenuContent'
import { ColumnResizer, ResizerHeight } from './ColumnResizer'
Expand Down Expand Up @@ -104,7 +104,7 @@ export const TableHeaderCellContents: React.FC<{
setMenuSuppressed,
resizerHeight
}) => {
const isTimestamp = header.headerGroup.id === ColumnType.TIMESTAMP
const isTimestamp = header.id === 'Created'
const columnIsResizing = header.column.getIsResizing()

useEffect(() => {
Expand Down
Loading