From 1ddbd76cd8b95c89954043c17a8be5203c0f94a7 Mon Sep 17 00:00:00 2001 From: Ashton Galloway Date: Fri, 1 Nov 2024 13:19:24 -0400 Subject: [PATCH 1/2] fix: cover select all edge cases this changes the select all logic. previously the select all prompt would appear when selecting more than the page limit's worth of objects across the list. if the selected count equaled the total rows in the result set, the clear selection prompt would appear. now, the select all prompt appears when the selection includes all visible rows in the result set, but the selection is not the entire result set. if the selection otherwise exceeds the page limit, we show the clear selection prompt. this fixes the select all prompt appearing when selecting single rows across pages, as well as the select all prompt not appearing when selecting all rows on the last page of results that does not hit the page limit. there was also an issue where, for searches and experiments, the select all prompt would appear when selecting 20 rows regardless of the page size, which is also fixed here. --- .../src/components/LoadableCount.test.tsx | 3 ++ webui/react/src/components/LoadableCount.tsx | 50 +++++++++++-------- .../src/components/Searches/Searches.tsx | 12 +---- webui/react/src/components/TableActionBar.tsx | 33 ++++++++---- .../src/pages/F_ExpList/F_ExperimentList.tsx | 13 ++--- webui/react/src/pages/FlatRuns/FlatRuns.tsx | 33 ++++++------ 6 files changed, 78 insertions(+), 66 deletions(-) diff --git a/webui/react/src/components/LoadableCount.test.tsx b/webui/react/src/components/LoadableCount.test.tsx index 13c28789045..91adf9122a6 100644 --- a/webui/react/src/components/LoadableCount.test.tsx +++ b/webui/react/src/components/LoadableCount.test.tsx @@ -1,5 +1,6 @@ import { render, screen } from '@testing-library/react'; import { Loaded, NotLoaded } from 'hew/utils/loadable'; +import { noop } from 'lodash'; import LoadableCount from './LoadableCount'; @@ -24,9 +25,11 @@ const setup = (totalCount: number, selectedCount?: number, loaded?: boolean) => const total = loaded ? Loaded(totalCount) : NotLoaded; render( , ); diff --git a/webui/react/src/components/LoadableCount.tsx b/webui/react/src/components/LoadableCount.tsx index 357c17d3d9a..c1041628266 100644 --- a/webui/react/src/components/LoadableCount.tsx +++ b/webui/react/src/components/LoadableCount.tsx @@ -1,4 +1,5 @@ import Button from 'hew/Button'; +import { HandleSelectionChangeType } from 'hew/DataGrid/DataGrid'; import { Loadable } from 'hew/utils/loadable'; import { useMemo } from 'react'; @@ -7,24 +8,23 @@ import { pluralizer } from 'utils/string'; import css from './LoadableCount.module.scss'; +export type SelectionAction = 'SELECT_ALL' | 'CLEAR_SELECTION' | 'NONE'; interface Props { total: Loadable; labelSingular: string; labelPlural: string; - onActualSelectAll?: () => void; - onClearSelect?: () => void; - pageSize?: number; selectedCount: number; + selectionAction: SelectionAction; + handleSelectionChange: HandleSelectionChangeType; } const LoadableCount: React.FC = ({ total, labelPlural, labelSingular, - onActualSelectAll, - onClearSelect, - pageSize = 20, selectedCount, + selectionAction, + handleSelectionChange, }: Props) => { const isMobile = useMobile(); @@ -51,25 +51,31 @@ const LoadableCount: React.FC = ({ const actualSelectAll = useMemo(() => { return Loadable.match(total, { _: () => null, - Loaded: (loadedTotal) => { - if (onActualSelectAll && selectedCount >= pageSize && selectedCount < loadedTotal) { - return ( - - ); - } else if (onClearSelect && (selectedCount >= pageSize || selectedCount === loadedTotal)) { - return ( - - ); + Loaded: () => { + switch (selectionAction) { + case 'SELECT_ALL': { + const onClick = () => handleSelectionChange('add-all'); + return ( + + ); + } + case 'CLEAR_SELECTION': { + const onClick = () => handleSelectionChange('remove-all'); + return ( + + ); + } + case 'NONE': { + return null; + } } - - return null; }, }); - }, [labelPlural, onActualSelectAll, onClearSelect, pageSize, selectedCount, total]); + }, [labelPlural, handleSelectionChange, selectionAction, total]); if (!isMobile) { return ( diff --git a/webui/react/src/components/Searches/Searches.tsx b/webui/react/src/components/Searches/Searches.tsx index 95b3cfd2d96..f5dc484792c 100644 --- a/webui/react/src/components/Searches/Searches.tsx +++ b/webui/react/src/components/Searches/Searches.tsx @@ -637,14 +637,6 @@ const Searches: React.FC = ({ project }) => { users, ]); - const handleActualSelectAll = useCallback(() => { - handleSelectionChange?.('add-all'); - }, [handleSelectionChange]); - - const handleClearSelect = useCallback(() => { - handleSelectionChange?.('remove-all'); - }, [handleSelectionChange]); - const handleHeaderClick = useCallback( (columnId: string): void => { if (columnId === MULTISELECT) { @@ -787,8 +779,10 @@ const Searches: React.FC = ({ project }) => { columnGroups={[V1LocationType.EXPERIMENT]} entityCopy="Show searches…" formStore={formStore} + handleSelectionChange={handleSelectionChange} initialVisibleColumns={columnsIfLoaded} isOpenFilter={isOpenFilter} + isRangeSelected={isRangeSelected} labelPlural="searches" labelSingular="search" pageSize={settings.pageLimit} @@ -803,8 +797,6 @@ const Searches: React.FC = ({ project }) => { total={total} onActionComplete={handleActionComplete} onActionSuccess={handleActionSuccess} - onActualSelectAll={handleActualSelectAll} - onClearSelect={handleClearSelect} onIsOpenFilterChange={handleIsOpenFilterChange} onRowHeightChange={handleRowHeightChange} onSortChange={handleSortChange} diff --git a/webui/react/src/components/TableActionBar.tsx b/webui/react/src/components/TableActionBar.tsx index dff835730fd..b49c5fc6ae0 100644 --- a/webui/react/src/components/TableActionBar.tsx +++ b/webui/react/src/components/TableActionBar.tsx @@ -1,6 +1,6 @@ import Button from 'hew/Button'; import Column from 'hew/Column'; -import { Sort } from 'hew/DataGrid/DataGrid'; +import { HandleSelectionChangeType, Sort } from 'hew/DataGrid/DataGrid'; import Dropdown, { MenuItem } from 'hew/Dropdown'; import Icon, { IconName } from 'hew/Icon'; import { useModal } from 'hew/Modal'; @@ -57,7 +57,7 @@ import { capitalizeWord } from 'utils/string'; import { openCommandResponse } from 'utils/wait'; import { FilterFormSet } from './FilterForm/components/type'; -import LoadableCount from './LoadableCount'; +import LoadableCount, { SelectionAction } from './LoadableCount'; import css from './TableActionBar.module.scss'; const batchActions = [ @@ -91,14 +91,14 @@ const actionIcons: Record = { interface Props { compareViewOn?: boolean; formStore: FilterFormStore; + handleSelectionChange: HandleSelectionChangeType; heatmapBtnVisible?: boolean; heatmapOn?: boolean; initialVisibleColumns: string[]; isOpenFilter: boolean; + isRangeSelected: (range: [number, number]) => boolean; onActionComplete?: () => Promise; onActionSuccess?: (action: BatchAction, successfulIds: number[]) => void; - onActualSelectAll?: () => void; - onClearSelect?: () => void; onComparisonViewToggle?: () => void; onHeatmapToggle?: (heatmapOn: boolean) => void; onIsOpenFilterChange?: (value: boolean) => void; @@ -106,7 +106,7 @@ interface Props { onSortChange?: (sorts: Sort[]) => void; onVisibleColumnChange?: (newColumns: string[], pinnedCount?: number) => void; onHeatmapSelectionRemove?: (id: string) => void; - pageSize?: number; + pageSize: number; project: Project; projectColumns: Loadable; rowHeight: RowHeight; @@ -129,14 +129,14 @@ const TableActionBar: React.FC = ({ compareViewOn, formStore, tableFilterString, + handleSelectionChange, heatmapBtnVisible, heatmapOn, initialVisibleColumns, isOpenFilter, + isRangeSelected, onActionComplete, onActionSuccess, - onActualSelectAll, - onClearSelect, onComparisonViewToggle, onHeatmapToggle, onIsOpenFilterChange, @@ -412,6 +412,20 @@ const TableActionBar: React.FC = ({ }, [] as MenuItem[]); }, [availableBatchActions]); + const selectionAction: SelectionAction = useMemo(() => { + return total.match({ + _: () => 'NONE' as const, + Loaded: (loadedTotal) => { + if (isRangeSelected([0, pageSize]) && selectionSize < loadedTotal) { + return 'SELECT_ALL'; + } else if (selectionSize > pageSize) { + return 'CLEAR_SELECTION'; + } + return 'NONE'; + }, + }); + }, [isRangeSelected, selectionSize, pageSize, total]); + return (
@@ -455,13 +469,12 @@ const TableActionBar: React.FC = ({ )} diff --git a/webui/react/src/pages/F_ExpList/F_ExperimentList.tsx b/webui/react/src/pages/F_ExpList/F_ExperimentList.tsx index c816d04ec04..0314b426cbd 100644 --- a/webui/react/src/pages/F_ExpList/F_ExperimentList.tsx +++ b/webui/react/src/pages/F_ExpList/F_ExperimentList.tsx @@ -487,14 +487,6 @@ const F_ExperimentList: React.FC = ({ project }) => { [handleSelectionChange, openToast], ); - const handleActualSelectAll = useCallback(() => { - handleSelectionChange?.('add-all'); - }, [handleSelectionChange]); - - const handleClearSelect = useCallback(() => { - handleSelectionChange?.('remove-all'); - }, [handleSelectionChange]); - const handleContextMenuComplete = useCallback( (action: ExperimentAction, id: number, data?: Partial) => handleActionSuccess(action, [id], data), @@ -987,12 +979,15 @@ const F_ExperimentList: React.FC = ({ project }) => { compareViewOn={settings.compare} entityCopy="Show experiments…" formStore={formStore} + handleSelectionChange={handleSelectionChange} heatmapBtnVisible={heatmapBtnVisible} heatmapOn={settings.heatmapOn} initialVisibleColumns={columnsIfLoaded} isOpenFilter={isOpenFilter} + isRangeSelected={isRangeSelected} labelPlural="experiments" labelSingular="experiment" + pageSize={settings.pageLimit} pinnedColumnsCount={settings.pinnedColumnsCount} project={project} projectColumns={projectColumns} @@ -1005,8 +1000,6 @@ const F_ExperimentList: React.FC = ({ project }) => { total={total} onActionComplete={handleActionComplete} onActionSuccess={handleActionSuccess} - onActualSelectAll={handleActualSelectAll} - onClearSelect={handleClearSelect} onComparisonViewToggle={handleToggleComparisonView} onHeatmapSelectionRemove={(id) => { const newSelection = settings.heatmapSkipped.filter((s) => s !== id); diff --git a/webui/react/src/pages/FlatRuns/FlatRuns.tsx b/webui/react/src/pages/FlatRuns/FlatRuns.tsx index e9fbc37c353..389c01b08ea 100644 --- a/webui/react/src/pages/FlatRuns/FlatRuns.tsx +++ b/webui/react/src/pages/FlatRuns/FlatRuns.tsx @@ -46,7 +46,7 @@ import { SpecialColumnNames, } from 'components/FilterForm/components/type'; import TableFilter from 'components/FilterForm/TableFilter'; -import LoadableCount from 'components/LoadableCount'; +import LoadableCount, { SelectionAction } from 'components/LoadableCount'; import MultiSortMenu, { EMPTY_SORT, sortMenuItemsForColumn } from 'components/MultiSortMenu'; import { OptionsMenu, RowHeight } from 'components/OptionsMenu'; import { @@ -779,21 +779,13 @@ const FlatRuns: React.FC = ({ projectId, workspaceId, searchId }) => { [updateSettings], ); - const handleActualSelectAll = useCallback(() => { - handleSelectionChange?.('add-all'); - }, [handleSelectionChange]); - - const handleClearSelect = useCallback(() => { - handleSelectionChange?.('remove-all'); - }, [handleSelectionChange]); - const handleHeaderClick = useCallback( (columnId: string): void => { if (columnId === MULTISELECT) { if (isRangeSelected([0, settings.pageLimit])) { - handleSelectionChange?.('remove', [0, settings.pageLimit]); + handleSelectionChange('remove', [0, settings.pageLimit]); } else { - handleSelectionChange?.('add', [0, settings.pageLimit]); + handleSelectionChange('add', [0, settings.pageLimit]); } } }, @@ -975,6 +967,20 @@ const FlatRuns: React.FC = ({ projectId, workspaceId, searchId }) => { }; }, [canceler, stopPolling]); + const selectionAction: SelectionAction = useMemo(() => { + return total.match({ + _: () => 'NONE' as const, + Loaded: (loadedTotal) => { + if (isRangeSelected([0, settings.pageLimit]) && selectionSize < loadedTotal) { + return 'SELECT_ALL'; + } else if (selectionSize > settings.pageLimit) { + return 'CLEAR_SELECTION'; + } + return 'NONE'; + }, + }); + }, [isRangeSelected, selectionSize, settings.pageLimit, total]); + return (
@@ -1029,13 +1035,12 @@ const FlatRuns: React.FC = ({ projectId, workspaceId, searchId }) => { onActionComplete={onActionComplete} /> From c48f0163a6bf82b6fdaefb6ffa81af35b10a5b40 Mon Sep 17 00:00:00 2001 From: Ashton Galloway Date: Tue, 5 Nov 2024 12:52:40 -0500 Subject: [PATCH 2/2] fix deselect logic for e2e --- .../src/e2e/models/components/TableActionBar.ts | 1 + webui/react/src/e2e/tests/experimentList.spec.ts | 13 +++++++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/webui/react/src/e2e/models/components/TableActionBar.ts b/webui/react/src/e2e/models/components/TableActionBar.ts index 17acec62672..433f339ea1f 100644 --- a/webui/react/src/e2e/models/components/TableActionBar.ts +++ b/webui/react/src/e2e/models/components/TableActionBar.ts @@ -26,6 +26,7 @@ export class TableActionBar extends NamedComponent { heatmapToggle = new BaseComponent({ parent: this, selector: '[data-test="heatmapToggle"]' }); compare = new BaseComponent({ parent: this, selector: '[data-test="compare"]' }); clearSelection = new BaseComponent({ parent: this, selector: '[data-test="clear-selection"]' }); + selectAll = new BaseComponent({ parent: this, selector: '[data-test="select-all"]' }); // TODO a bunch of modals } diff --git a/webui/react/src/e2e/tests/experimentList.spec.ts b/webui/react/src/e2e/tests/experimentList.spec.ts index 8ae7f66d01a..1d122c84663 100644 --- a/webui/react/src/e2e/tests/experimentList.spec.ts +++ b/webui/react/src/e2e/tests/experimentList.spec.ts @@ -65,10 +65,15 @@ test.describe('Experiment List', () => { const count = await getCount(); if (count !== 0) { await grid.headRow.clickSelectHeader(); - const isClearSelectionVisible = - await projectDetailsPage.f_experimentList.tableActionBar.clearSelection.pwLocator.isVisible(); - if (isClearSelectionVisible) { - await projectDetailsPage.f_experimentList.tableActionBar.clearSelection.pwLocator.click(); + const selectAllButton = projectDetailsPage.f_experimentList.tableActionBar.selectAll; + const clearAllButton = projectDetailsPage.f_experimentList.tableActionBar.clearSelection; + if (await selectAllButton.pwLocator.isVisible()) { + await selectAllButton.pwLocator.click(); + await clearAllButton.pwLocator.click(); + } else if (await clearAllButton.pwLocator.isVisible()) { + await clearAllButton.pwLocator.click(); + } else { + await grid.headRow.clickSelectHeader(); } } });