-
Notifications
You must be signed in to change notification settings - Fork 29
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
Clean up table head styles #3674
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it's just me, but |
||
<span>{name}</span> | ||
</div> | ||
</OverflowHoverTooltip> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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' | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
}, | ||
headerDropTargetId === header.id && styles.headerCellDropTarget | ||
[styles.firstLevelHeaderCell]: isFirstLevelHeader(header.column.id), | ||
[styles.leafHeaderCell]: header.subHeaders === undefined, | ||
[styles.oneRowHeaderCell]: onlyOneLine, | ||
[styles.dropTargetHeaderCell]: headerDropTargetId === header.id | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since all of these object classes accomplish the same thing (applying extra styles to |
||
} | ||
), | ||
style: { | ||
position: undefined | ||
|
@@ -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} | ||
|
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' | ||
|
@@ -104,7 +104,7 @@ export const TableHeaderCellContents: React.FC<{ | |
setMenuSuppressed, | ||
resizerHeight | ||
}) => { | ||
const isTimestamp = header.headerGroup.id === ColumnType.TIMESTAMP | ||
const isTimestamp = header.id === 'Created' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Funnily enough, there were two attempts in the code to move the timestamps icon menu to the right:
The first way wasn't technically working so the second way was the one being applied to the actual cell. I decided to go the first way so I fixed the |
||
const columnIsResizing = header.column.getIsResizing() | ||
|
||
useEffect(() => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
sortingHeaderCellDesc
andsortingHeaderCellDesc
styles aren't actually being used outside of tests. I deleted the classes and corresponding tests, but if this was intentional for testing purposes, I can add them back :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the behaviour under test elsewhere (maybe that an arrow/indicator is shown)? If not then should add another test with maybe a data-testid. No reason to keep the class around just for this.