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.

Sign up for GitHub

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

Jump to bottom

fix: improve sorting for multi-type columns #10123

Merged
merged 2 commits into from
Oct 29, 2024
Merged

fix: improve sorting for multi-type columns #10123

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aff0f22
fix: improve sorting for multi-type columns
ashtonG Oct 24, 2024
4fde32e
fix title for tests
ashtonG Oct 29, 2024
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
61 changes: 56 additions & 5 deletions webui/react/src/components/MultiSortMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
import Badge from 'hew/Badge';
import Button from 'hew/Button';
import { DirectionType, Sort, validSort } from 'hew/DataGrid/DataGrid';
import Dropdown, { MenuItem } from 'hew/Dropdown';
import Icon from 'hew/Icon';
import Select from 'hew/Select';
import { Loadable } from 'hew/utils/loadable';
import { groupBy, mapValues } from 'lodash';
import { Fragment, useMemo } from 'react';

import { runColumns } from 'pages/FlatRuns/columns';
import { V1ColumnType } from 'services/api-ts-sdk';
import { ProjectColumn } from 'types';
import { removeColumnTypePrefix } from 'utils/flatRun';

import css from './MultiSortMenu.module.scss';

Expand All @@ -25,6 +30,7 @@ interface MultiSortRowProps {
onChange?: (sort: Sort) => void;
onRemove?: () => void;
bannedSortColumns: Set<string>;
typeMap: Record<string, V1ColumnType[]>;
}
interface DirectionOptionsProps {
onChange?: (direction: DirectionType) => void;
Expand All @@ -36,6 +42,7 @@ interface ColumnOptionsProps {
onChange?: (column: string) => void;
value?: string;
bannedSortColumns: Set<string>;
typeMap: Record<string, V1ColumnType[]>;
}

export const optionsByColumnType = {
Expand Down Expand Up @@ -146,6 +153,7 @@ const ColumnOptions: React.FC<ColumnOptionsProps> = ({
columns,
value,
bannedSortColumns,
typeMap,
}) => (
<Select
autoFocus
Expand All @@ -154,10 +162,29 @@ const ColumnOptions: React.FC<ColumnOptionsProps> = ({
loading={Loadable.isNotLoaded(columns)}
options={Loadable.getOrElse([], columns)
.filter((c) => !bannedSortColumns.has(c.column))
.map((c) => ({
label: c.displayName || c.column,
value: c.column,
}))}
.map((c) => {
const badges = typeMap[c.column].map((type) => {
const copy =
(runColumns as readonly string[]).includes(c.column) &&
type === V1ColumnType.UNSPECIFIED
? 'BOOLEAN'
: removeColumnTypePrefix(type);
return (
<Fragment key={type}>
<Badge text={copy} />{' '}
</Fragment>
);
});
const label = (
<>
{c.displayName || c.column} {badges}
</>
Copy link
Contributor

Choose a reason for hiding this comment

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

tbh... idk if it's better to show the badges grouped or individually, for instance, the first time that I saw it I thought that it was a bug 😅... maybe that's something to keep in mind when we add documentation for the release

);
return {
label,
value: c.column,
};
})}
placeholder="Select column"
value={value}
width="100%"
Expand All @@ -171,6 +198,7 @@ const MultiSortRow: React.FC<MultiSortRowProps> = ({
onChange,
onRemove,
bannedSortColumns,
typeMap,
}) => {
const valueType =
Loadable.getOrElse([], columns).find((c) => c.column === sort.column)?.type ||
Expand All @@ -181,6 +209,7 @@ const MultiSortRow: React.FC<MultiSortRowProps> = ({
<ColumnOptions
bannedSortColumns={bannedSortColumns}
columns={columns}
typeMap={typeMap}
value={sort.column}
onChange={(column) => onChange?.({ ...sort, column })}
/>
Expand Down Expand Up @@ -224,14 +253,35 @@ const MultiSort: React.FC<MultiSortProps> = ({ sorts, columns, onChange, bannedS
window.document.body.dispatchEvent(new Event('mousedown', { bubbles: true }));
onChange?.([EMPTY_SORT]);
};
// replace duplicate columns with a single unspecified column for copy
// reasons. maintain types so we can display in the select
const [mergedColumns, typeMap] = useMemo(() => {
const loadableTuple = columns.map((c) => {
const columnGroups = groupBy(c, 'column');
const fixedColumns = Object.values(columnGroups).flatMap((group) => {
if (group.length > 1) {
return [
{
...group[0],
type: V1ColumnType.UNSPECIFIED,
},
];
}
return group;
});
const typeMap = mapValues(columnGroups, (group) => group.map((column) => column.type));
return [fixedColumns, typeMap] as const;
});
return [loadableTuple.map((l) => l[0]), loadableTuple.map((l) => l[1]).getOrElse({})];
}, [columns]);

return (
<div className={css.base} data-test-component="multiSort">
<div>{SORT_MENU_TITLE}</div>
<div className={css.rows} data-test="rows">
{sorts.map((sort, idx) => {
const seenColumns = sorts.slice(0, idx).map((s) => s.column);
const columnOptions = Loadable.map(columns, (cols) =>
const columnOptions = mergedColumns.map((cols) =>
cols.filter((c) => !seenColumns.includes(c.column)),
);
return (
Expand All @@ -240,6 +290,7 @@ const MultiSort: React.FC<MultiSortProps> = ({ sorts, columns, onChange, bannedS
columns={columnOptions}
key={sort.column || idx}
sort={sort}
typeMap={typeMap}
onChange={makeOnRowChange(idx)}
onRemove={makeOnRowRemove(idx)}
/>
Expand Down
39 changes: 28 additions & 11 deletions webui/react/src/pages/FlatRuns/FlatRuns.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import Pagination from 'hew/Pagination';
import Row from 'hew/Row';
import { useToast } from 'hew/Toast';
import { Loadable, Loaded, NotLoaded } from 'hew/utils/loadable';
import { groupBy, keyBy, mapValues } from 'lodash';
import { useObservable } from 'micro-observables';
import React, { useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState } from 'react';
import { v4 as uuidv4 } from 'uuid';
Expand Down Expand Up @@ -79,7 +80,6 @@ import {
DetailedUser,
FlatRun,
FlatRunAction,
ProjectColumn,
RunState,
SelectionType as SelectionState,
} from 'types';
Expand Down Expand Up @@ -187,6 +187,7 @@ const FlatRuns: React.FC<Props> = ({ projectId, workspaceId, searchId }) => {
const [runs, setRuns] = useState<Loadable<FlatRun>[]>(INITIAL_LOADING_RUNS);

const [sorts, setSorts] = useState<Sort[]>([EMPTY_SORT]);

const sortString = useMemo(() => makeSortString(sorts.filter(validSort.is)), [sorts]);
const loadableFormset = useObservable(formStore.formset);
const rootFilterChildren: Array<FormGroup | FormField> = Loadable.match(loadableFormset, {
Expand Down Expand Up @@ -233,6 +234,24 @@ const FlatRuns: React.FC<Props> = ({ projectId, workspaceId, searchId }) => {
}
}, [projectId]);

// expand sorts to include types from metadata columns so the column ids match
const expandedSorts = useMemo(() => {
return projectColumns.match({
_: () => sorts,
Loaded(pc) {
const groupedColumns = groupBy(pc, (c) => c.column);
const columnToExpandedColumns = mapValues(groupedColumns, (cols) =>
cols.map((c) => formatColumnKey(c)),
);
return sorts
.filter(validSort.is)
.flatMap((sort) =>
columnToExpandedColumns[sort.column].map((ec) => ({ ...sort, column: ec })),
);
},
});
}, [sorts, projectColumns]);

const arrayTypeColumns = useMemo(() => {
const arrayTypeColumns = projectColumns
.getOrElse([])
Expand Down Expand Up @@ -317,13 +336,11 @@ const FlatRuns: React.FC<Props> = ({ projectId, workspaceId, searchId }) => {
return [...STATIC_COLUMNS, ...settings.columns.slice(0, settings.pinnedColumnsCount)];
}, [settings.columns, settings.pinnedColumnsCount]);

const projectColumnsMap = useMemo(() => {
return projectColumns.map((columns) => keyBy(columns, formatColumnKey));
}, [projectColumns]);

const columns: ColumnDef<FlatRun>[] = useMemo(() => {
const projectColumnsMap: Loadable<Record<string, ProjectColumn>> = Loadable.map(
projectColumns,
(columns) => {
return columns.reduce((acc, col) => ({ ...acc, [formatColumnKey(col)]: col }), {});
},
);
const columnDefs = getColumnDefs({
appTheme,
columnWidths: settings.columnWidths,
Expand Down Expand Up @@ -480,7 +497,7 @@ const FlatRuns: React.FC<Props> = ({ projectId, workspaceId, searchId }) => {
appTheme,
columnsIfLoaded,
isDarkMode,
projectColumns,
projectColumnsMap,
projectHeatmap,
selection.rows,
settings.columnWidths,
Expand Down Expand Up @@ -905,7 +922,7 @@ const FlatRuns: React.FC<Props> = ({ projectId, workspaceId, searchId }) => {
return items;
}

const column = Loadable.getOrElse([], projectColumns).find((c) => c.column === columnId);
const column = projectColumnsMap.getOrElse({})[columnId];
const isPinned = colIdx <= settings.pinnedColumnsCount + STATIC_COLUMNS.length - 1;
const items: MenuItem[] = [
// Column is pinned if the index is inside of the frozen columns
Expand Down Expand Up @@ -1052,7 +1069,7 @@ const FlatRuns: React.FC<Props> = ({ projectId, workspaceId, searchId }) => {
[
bannedFilterColumns,
bannedSortColumns,
projectColumns,
projectColumnsMap,
settings.pinnedColumnsCount,
settings.selection,
settings.pageLimit,
Expand Down Expand Up @@ -1212,7 +1229,7 @@ const FlatRuns: React.FC<Props> = ({ projectId, workspaceId, searchId }) => {
}}
rowHeight={rowHeightMap[globalSettings.rowHeight as RowHeight]}
selection={selection}
sorts={sorts}
sorts={expandedSorts}
staticColumns={STATIC_COLUMNS}
onColumnResize={handleColumnWidthChange}
onColumnsOrderChange={handleColumnsOrderChange}
Expand Down
Loading