diff --git a/static/app/components/replays/virtualizedGrid/headerCell.tsx b/static/app/components/replays/virtualizedGrid/headerCell.tsx index 7291ecfa208a..af744b55ed3f 100644 --- a/static/app/components/replays/virtualizedGrid/headerCell.tsx +++ b/static/app/components/replays/virtualizedGrid/headerCell.tsx @@ -3,7 +3,8 @@ import styled from '@emotion/styled'; import {Tooltip} from '@sentry/scraps/tooltip'; -import {IconArrow, IconInfo} from 'sentry/icons'; +import {SortableHeaderCell} from 'sentry/components/tables/sortableHeaderCell'; +import {IconInfo} from 'sentry/icons'; type BaseRecord = Record; export interface SortConfig { @@ -19,11 +20,12 @@ type Props = { sortConfig: SortConfig; style: CSSProperties; tooltipTitle: undefined | ReactNode; - ref?: React.Ref; }; const StyledIconInfo = styled(IconInfo)` - display: block; + margin-left: ${p => p.theme.space.xs}; + margin-top: 1px; + vertical-align: text-top; `; function CatchClicks({children}: {children: ReactNode}) { @@ -37,28 +39,24 @@ export function HeaderCell({ sortConfig, style, tooltipTitle, - ref, }: Props) { return ( - handleSort(field)} ref={ref}> + handleSort(field)} + style={style} + > {label} {tooltipTitle ? ( {tooltipTitle}}> ) : null} - ); } -const HeaderButton = styled('button')` - border: 0; +const HeaderButton = styled(SortableHeaderCell)` border-bottom: 1px solid ${p => p.theme.tokens.border.primary}; background: ${p => p.theme.tokens.background.secondary}; color: ${p => p.theme.tokens.content.secondary}; @@ -66,18 +64,9 @@ const HeaderButton = styled('button')` font-size: ${p => p.theme.font.size.sm}; font-weight: ${p => p.theme.font.weight.sans.medium}; line-height: 16px; - text-align: unset; text-transform: uppercase; - white-space: nowrap; width: 100%; - display: flex; - align-items: center; - justify-content: space-between; padding: ${p => p.theme.space.xs} ${p => p.theme.space.md} ${p => p.theme.space.xs} ${p => p.theme.space.lg}; - - svg { - margin-left: ${p => p.theme.space['2xs']}; - } `; diff --git a/static/app/components/tables/gridEditable/index.spec.tsx b/static/app/components/tables/gridEditable/index.spec.tsx new file mode 100644 index 000000000000..c0ee6b94a95c --- /dev/null +++ b/static/app/components/tables/gridEditable/index.spec.tsx @@ -0,0 +1,60 @@ +import {render, screen} from 'sentry-test/reactTestingLibrary'; + +import type {GridColumnOrder} from 'sentry/components/tables/gridEditable'; +import {GridEditable} from 'sentry/components/tables/gridEditable'; + +type Row = {count: number; name: string}; + +const DATA: Row[] = [{name: 'first', count: 1}]; + +const COLUMN_ORDER: Array> = [ + {key: 'name', name: 'Name'}, + {key: 'count', name: 'Count'}, +]; + +describe('GridEditable', () => { + it('announces descending when a column is sorted descending', () => { + render( + + ); + + expect(screen.getByRole('columnheader', {name: 'Count'})).toHaveAttribute( + 'aria-sort', + 'descending' + ); + expect(screen.getByRole('columnheader', {name: 'Name'})).not.toHaveAttribute( + 'aria-sort' + ); + }); + + it('announces ascending when a column is sorted ascending', () => { + render( + + ); + + expect(screen.getByRole('columnheader', {name: 'Name'})).toHaveAttribute( + 'aria-sort', + 'ascending' + ); + }); + + it('announces no sort when the table is unsorted', () => { + render( + + ); + + expect(screen.getByRole('columnheader', {name: 'Count'})).not.toHaveAttribute( + 'aria-sort' + ); + }); +}); diff --git a/static/app/components/tables/gridEditable/index.tsx b/static/app/components/tables/gridEditable/index.tsx index 2ee86ffce756..e804f457acf5 100644 --- a/static/app/components/tables/gridEditable/index.tsx +++ b/static/app/components/tables/gridEditable/index.tsx @@ -6,6 +6,7 @@ import InteractionStateLayer from '@sentry/scraps/interactionStateLayer'; import {GridEditableEmptyData} from 'sentry/components/tables/gridEditable/GridEditableEmptyData'; import {GridEditableError} from 'sentry/components/tables/gridEditable/GridEditableError'; import {GridEditableLoading} from 'sentry/components/tables/gridEditable/GridEditableLoading'; +import {getAriaSort} from 'sentry/components/tables/sortableHeaderCell'; import {onRenderCallback, Profiler} from 'sentry/utils/performanceForSentry'; import { @@ -91,8 +92,8 @@ type GridEditableProps< * based on this 3 main props. * * - `columnOrder` determines the columns to show, from left to right - * - `columnSortBy` is not used at the moment, however it might be better to - * move sorting into Grid for performance + * - `columnSortBy` tells each header cell which sort state to announce; the + * sort itself is still performed by the parent component */ title?: ReactNode; }; @@ -299,6 +300,9 @@ export function GridEditable< // 1 levels under GridHeadCell props.columnOrder.map((column, i) => ( sort.key === column.key)?.order + )} data-test-id="grid-head-cell" key={`${i}.${String(column.key)}`} isFirst={i === 0} diff --git a/static/app/components/tables/gridEditable/sortLink.tsx b/static/app/components/tables/gridEditable/sortLink.tsx index 878f999425b7..f4aa7c1d0efe 100644 --- a/static/app/components/tables/gridEditable/sortLink.tsx +++ b/static/app/components/tables/gridEditable/sortLink.tsx @@ -5,15 +5,14 @@ import type {LocationDescriptorObject} from 'history'; import {Link} from '@sentry/scraps/link'; import type {ColumnAlign} from 'sentry/components/tables/gridEditable'; +import type {SortDirection} from 'sentry/components/tables/sortableHeaderCell'; import {IconArrow} from 'sentry/icons'; import {useNavigate} from 'sentry/utils/useNavigate'; -export type ColumnSort = 'desc' | 'asc' | undefined; - type Props = { align: ColumnAlign; canSort: boolean; - direction: ColumnSort; + direction: SortDirection | undefined; title: React.ReactNode; generateSortLink?: () => LocationDescriptorObject | undefined; onClick?: (e: React.MouseEvent) => void; diff --git a/static/app/components/tables/simpleTable/index.spec.tsx b/static/app/components/tables/simpleTable/index.spec.tsx index b304b0cd1b39..1aedd6ce24f3 100644 --- a/static/app/components/tables/simpleTable/index.spec.tsx +++ b/static/app/components/tables/simpleTable/index.spec.tsx @@ -1,4 +1,4 @@ -import {render, screen, within} from 'sentry-test/reactTestingLibrary'; +import {render, screen, userEvent, within} from 'sentry-test/reactTestingLibrary'; import {SimpleTable} from 'sentry/components/tables/simpleTable'; @@ -38,4 +38,56 @@ describe('SimpleTable component', () => { expect(within(row2).getByRole('cell', {name: '4'})).toBeInTheDocument(); expect(within(row2).getByRole('cell', {name: '5'})).toBeInTheDocument(); }); + + it('announces the sort direction when a column is sorted', () => { + render( + + + + A + + B + + + ); + + expect(screen.getByRole('columnheader', {name: 'A'})).toHaveAttribute( + 'aria-sort', + 'ascending' + ); + expect(screen.getByRole('columnheader', {name: 'B'})).not.toHaveAttribute( + 'aria-sort' + ); + }); + + it('keeps the interaction state layer a direct child of the header cell when sortable', () => { + render( + + + A + + + ); + + const header = screen.getByRole('columnheader', {name: 'A'}); + + expect(within(header).getByRole('presentation').parentElement).toBe(header); + }); + + it('sorts when a sortable header is clicked', async () => { + const handleSortClick = jest.fn(); + render( + + + + A + + + + ); + + await userEvent.click(screen.getByRole('columnheader', {name: 'A'})); + + expect(handleSortClick).toHaveBeenCalledTimes(1); + }); }); diff --git a/static/app/components/tables/simpleTable/index.tsx b/static/app/components/tables/simpleTable/index.tsx index c028e17f8c4d..9b02ec23c8bb 100644 --- a/static/app/components/tables/simpleTable/index.tsx +++ b/static/app/components/tables/simpleTable/index.tsx @@ -1,4 +1,5 @@ import type {ComponentProps, HTMLAttributes, RefObject} from 'react'; +import {Fragment} from 'react'; import {css} from '@emotion/react'; import type {Theme} from '@emotion/react'; import styled from '@emotion/styled'; @@ -7,7 +8,11 @@ import InteractionStateLayer from '@sentry/scraps/interactionStateLayer'; import {Flex} from '@sentry/scraps/layout'; import {Panel} from 'sentry/components/panels/panel'; -import {IconArrow} from 'sentry/icons'; +import { + getAriaSort, + SortableHeaderCell, + type SortDirection, +} from 'sentry/components/tables/sortableHeaderCell'; import {defined} from 'sentry/utils/defined'; interface TableProps extends HTMLAttributes { @@ -45,31 +50,23 @@ function HeaderCell({ children?: React.ReactNode; divider?: boolean; handleSortClick?: () => void; - sort?: 'asc' | 'desc'; + sort?: SortDirection; }) { - const isSorted = sort !== undefined; - const canSort = handleSortClick !== undefined; - return ( + {divider && } + {handleSortClick && } + + } role="columnheader" - as={canSort ? 'button' : 'div'} > - {divider && } - {canSort && } - {children} - {isSorted && ( - - )} + {children} ); } @@ -148,22 +145,15 @@ const HeaderDivider = styled('div')` height: 14px; `; -const ColumnHeaderCell = styled('div')<{isSorted?: boolean}>` - background: none; +const ColumnHeaderCell = styled(SortableHeaderCell)` outline: none; - border: none; padding: 0 ${p => p.theme.space.xl}; - text-transform: inherit; font-weight: ${p => p.theme.font.weight.sans.medium}; - text-align: left; font-size: ${p => p.theme.font.size.md}; color: ${p => p.theme.tokens.content.secondary}; position: relative; - display: flex; - align-items: center; justify-content: space-between; - gap: ${p => p.theme.space.md}; height: 100%; &:focus-visible { @@ -176,11 +166,9 @@ const ColumnHeaderCell = styled('div')<{isSorted?: boolean}>` } } - ${p => - p.isSorted && - css` - color: ${p.theme.tokens.content.primary}; - `} + &[aria-sort] { + color: ${p => p.theme.tokens.content.primary}; + } `; const rowLinkStyle = (p: {theme: Theme}) => css` @@ -198,18 +186,6 @@ const rowLinkStyle = (p: {theme: Theme}) => css` } `; -const SortIndicator = styled(IconArrow, { - shouldForwardProp: prop => prop !== 'isSorted', -})<{isSorted?: boolean}>` - visibility: hidden; - - ${p => - p.isSorted && - css` - visibility: visible; - `} -`; - const StyledEmptyMessage = styled('div')` grid-column: 1 / -1; min-height: 200px; diff --git a/static/app/components/tables/sortableHeaderCell.spec.tsx b/static/app/components/tables/sortableHeaderCell.spec.tsx new file mode 100644 index 000000000000..2bcf6fcaf049 --- /dev/null +++ b/static/app/components/tables/sortableHeaderCell.spec.tsx @@ -0,0 +1,56 @@ +import {render, screen, userEvent} from 'sentry-test/reactTestingLibrary'; + +import { + getAriaSort, + SortableHeaderCell, +} from 'sentry/components/tables/sortableHeaderCell'; + +describe('SortableHeaderCell', () => { + it('calls onSort when clicked', async () => { + const onSort = jest.fn(); + render(Duration); + + await userEvent.click(screen.getByRole('button', {name: 'Duration'})); + + expect(onSort).toHaveBeenCalledTimes(1); + }); + + it('renders a non-interactive cell when not given onSort', () => { + render(Duration); + + expect(screen.getByText('Duration')).toBeInTheDocument(); + expect(screen.queryByRole('button')).not.toBeInTheDocument(); + }); + + it('renders no indicator when the column is not sorted', () => { + render(Duration); + + expect(screen.queryByRole('img', {hidden: true})).not.toBeInTheDocument(); + }); + + it('points the indicator down when sorted descending', () => { + render( + + Duration + + ); + + expect(screen.getByRole('img', {hidden: true})).toHaveStyle({ + transform: 'scale(1, -1)', + }); + }); +}); + +describe('getAriaSort', () => { + it('returns ascending when sorted ascending', () => { + expect(getAriaSort('asc')).toBe('ascending'); + }); + + it('returns descending when sorted descending', () => { + expect(getAriaSort('desc')).toBe('descending'); + }); + + it('returns undefined when unsorted', () => { + expect(getAriaSort(undefined)).toBeUndefined(); + }); +}); diff --git a/static/app/components/tables/sortableHeaderCell.tsx b/static/app/components/tables/sortableHeaderCell.tsx new file mode 100644 index 000000000000..49335745dc62 --- /dev/null +++ b/static/app/components/tables/sortableHeaderCell.tsx @@ -0,0 +1,77 @@ +import type {HTMLAttributes, ReactNode} from 'react'; +import styled from '@emotion/styled'; + +import {Tooltip} from '@sentry/scraps/tooltip'; + +import {IconArrow} from 'sentry/icons'; + +export type SortDirection = 'asc' | 'desc'; + +export function getAriaSort( + direction: SortDirection | undefined +): 'ascending' | 'descending' | undefined { + switch (direction) { + case 'asc': + return 'ascending'; + case 'desc': + return 'descending'; + default: + return undefined; + } +} + +interface SortableHeaderCellProps extends HTMLAttributes { + children?: ReactNode; + direction?: SortDirection; + onSort?: () => void; + overlays?: ReactNode; +} + +export function SortableHeaderCell({ + children, + direction, + onSort, + overlays, + ...props +}: SortableHeaderCellProps) { + return ( + + {overlays} + + + + {direction && ( + + )} + + ); +} + +const Label = styled('div')` + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; +`; + +const HeaderCellContent = styled('div')<{type?: 'button'}>` + align-items: center; + background: none; + border: 0; + cursor: ${p => (p.onClick ? 'pointer' : 'default')}; + display: flex; + font: inherit; + gap: ${p => p.theme.space.xs}; + overflow: hidden; + padding: 0; + text-align: inherit; + text-transform: inherit; +`; diff --git a/static/app/views/explore/components/table.tsx b/static/app/views/explore/components/table.tsx index 923fd94d183d..3885ae954105 100644 --- a/static/app/views/explore/components/table.tsx +++ b/static/app/views/explore/components/table.tsx @@ -175,9 +175,3 @@ export const TableHeadCell = styled(GridHeadCell)<{align?: ColumnAlign}>` justify-content: ${p.align}; `} `; -export const TableHeadCellContent = styled('div')<{isFrozen?: boolean | undefined}>` - display: flex; - align-items: center; - gap: ${p => p.theme.space.xs}; - cursor: ${p => (p.isFrozen ? 'default' : 'pointer')}; -`; diff --git a/static/app/views/explore/logs/tables/logsInfiniteTable.tsx b/static/app/views/explore/logs/tables/logsInfiniteTable.tsx index 24c7a7d3f238..b6f18aee89ef 100644 --- a/static/app/views/explore/logs/tables/logsInfiniteTable.tsx +++ b/static/app/views/explore/logs/tables/logsInfiniteTable.tsx @@ -16,13 +16,16 @@ import {useVirtualizer} from '@tanstack/react-virtual'; import {Button} from '@sentry/scraps/button'; import {Flex, Stack} from '@sentry/scraps/layout'; -import {Tooltip} from '@sentry/scraps/tooltip'; import {FileSize} from 'sentry/components/fileSize'; import {LoadingIndicator} from 'sentry/components/loadingIndicator'; import {JumpButtons} from 'sentry/components/replays/jumpButtons'; import {useJumpButtons} from 'sentry/components/replays/useJumpButtons'; import {GridResizer} from 'sentry/components/tables/gridEditable/styles'; +import { + getAriaSort, + SortableHeaderCell, +} from 'sentry/components/tables/sortableHeaderCell'; import {IconArrow, IconWarning} from 'sentry/icons'; import {t, tct} from 'sentry/locale'; import type {Event} from 'sentry/types/event'; @@ -40,7 +43,6 @@ import {useLocation} from 'sentry/utils/useLocation'; import { TableBodyCell, TableHead, - TableHeadCellContent, TableRow, TableStatus, useTableStyles, @@ -756,9 +758,7 @@ function LogsTableHeader({ return ( - - - + {fields.map((field, index) => { const direction = sortBys.find(s => s.field === field)?.kind; @@ -783,12 +783,14 @@ function LogsTableHeader({ return ( - { @@ -804,24 +806,9 @@ function LogsTableHeader({ } } } - isFrozen={isFrozen} > - - {headerLabel} - - {defined(direction) && ( - - )} - + {headerLabel} + {index !== fields.length - 1 && ( ` font-size: ${p => p.theme.font.size.sm}; - white-space: nowrap; padding: ${p => p.noPadding ? 0 diff --git a/static/app/views/explore/replays/detail/errorList/errorHeaderCell.tsx b/static/app/views/explore/replays/detail/errorList/errorHeaderCell.tsx index d55eb12f1a67..9cd17bab07cc 100644 --- a/static/app/views/explore/replays/detail/errorList/errorHeaderCell.tsx +++ b/static/app/views/explore/replays/detail/errorList/errorHeaderCell.tsx @@ -12,7 +12,6 @@ type Props = { index: number; sortConfig: SortConfig; style: CSSProperties; - ref?: React.Ref; }; const COLUMNS: Array<{ @@ -29,11 +28,10 @@ const COLUMNS: Array<{ export const COLUMN_COUNT = COLUMNS.length; -export function ErrorHeaderCell({handleSort, index, sortConfig, style, ref}: Props) { +export function ErrorHeaderCell({handleSort, index, sortConfig, style}: Props) { const {field, label, tooltipTitle} = COLUMNS[index]!; return ( ; }; const COLUMNS: Array<{ @@ -57,11 +56,10 @@ const COLUMNS: Array<{ export const COLUMN_COUNT = COLUMNS.length; -export function NetworkHeaderCell({handleSort, index, sortConfig, style, ref}: Props) { +export function NetworkHeaderCell({handleSort, index, sortConfig, style}: Props) { const {field, label, tooltipTitle} = COLUMNS[index]!; return ( - - - + {visibleAggregateFields.map((aggregateField, i) => { // Hide column names before alignment is determined if (result.isPending) { @@ -180,24 +178,15 @@ export function AggregatesTable({ } return ( - - - - {label} - - {defined(direction) && ( - - )} - + + + {label} + {i !== visibleAggregateFields.length - 1 && ( - - - {label} - - {defined(direction) && ( - - )} - + + + {label} + {i !== visibleFields.length - 1 && ( { const {key, direction} = tableSort; - const getArrowDirection = (linkKey: SortBy): ColumnSort => { + const getArrowDirection = (linkKey: SortBy): SortDirection | undefined => { if (linkKey !== key) { return undefined; }