Skip to content

Commit cb48545

Browse files
mattseddonmattseddon
authored
Keep experiments table context menu consistent by showing disabled items (#3825)
* change implementation of context menu to use disabled items * rename hidden to disabled * update test titles * clean up table header context menu * test new behaviour * apply basic review feedback * use aria-disabled to filter click events
1 parent 4609a09 commit cb48545

File tree

15 files changed

+197
-130
lines changed

15 files changed

+197
-130
lines changed

‎webview/src/experiments/components/App.test.tsx‎

Lines changed: 94 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -723,13 +723,34 @@ describe('App', () => {
723723
advanceTimersByTime(100)
724724

725725
const menuitems = screen.getAllByRole('menuitem')
726-
expect(menuitems).toHaveLength(3)
726+
expect(menuitems).toHaveLength(6)
727+
expect(
728+
menuitems.filter(item => !item.className.includes('disabled'))
729+
).toHaveLength(3)
727730

728731
fireEvent.keyDown(paramsFileHeader, { bubbles: true, key: 'Escape' })
729732
expect(screen.queryAllByRole('menuitem')).toHaveLength(0)
730733
})
731734

732-
it('should have the same options in the empty placeholders', () => {
735+
it('should not close when a disabled item is clicked', () => {
736+
renderTableWithoutRunningExperiments()
737+
738+
const paramsFileHeader = screen.getByText('params.yaml')
739+
fireEvent.contextMenu(paramsFileHeader, { bubbles: true })
740+
741+
advanceTimersByTime(100)
742+
743+
const disabledMenuItem = screen
744+
.getAllByRole('menuitem')
745+
.find(item => item.className.includes('disabled'))
746+
747+
expect(disabledMenuItem).toBeDefined()
748+
749+
disabledMenuItem && fireEvent.click(disabledMenuItem, { bubbles: true })
750+
expect(screen.queryAllByRole('menuitem')).toHaveLength(6)
751+
})
752+
753+
it('should have the same enabled options in the empty placeholders', () => {
733754
renderTableWithPlaceholder()
734755
const header = screen.getByTestId('header-Created')
735756
const placeholders = screen.getAllByTestId(/header-Created.+placeholder/)
@@ -742,6 +763,7 @@ describe('App', () => {
742763
jest.advanceTimersByTime(100)
743764
const menuitems = screen
744765
.getAllByRole('menuitem')
766+
.filter(item => !item.className.includes('disabled'))
745767
.map(item => item.textContent)
746768

747769
expect(menuitems).toStrictEqual([
@@ -755,7 +777,7 @@ describe('App', () => {
755777
}
756778
})
757779

758-
it('should have the same options in the empty placeholders of the Experiment column', () => {
780+
it('should have the same enabled options in the empty placeholders of the Experiment column', () => {
759781
renderTableWithPlaceholder()
760782
const header = screen.getByTestId('header-id')
761783
const placeholders = screen.getAllByTestId(/header-id.+placeholder/)
@@ -768,6 +790,7 @@ describe('App', () => {
768790
jest.advanceTimersByTime(100)
769791
const menuitems = screen
770792
.getAllByRole('menuitem')
793+
.filter(item => !item.className.includes('disabled'))
771794
.map(item => item.textContent)
772795

773796
expect(menuitems).toStrictEqual(['Set Max Header Height'])
@@ -821,27 +844,31 @@ describe('App', () => {
821844
expect(menu).toBeDefined()
822845
})
823846

824-
it('should present the correct options for the workspace row with no checkpoints', () => {
847+
it('should enable the correct options for the workspace row with no checkpoints', () => {
825848
renderTableWithPlaceholder()
826849

827850
const target = screen.getByTestId('workspace-row')
828851
fireEvent.contextMenu(target, { bubbles: true })
829852

830853
advanceTimersByTime(100)
831854
const menuitems = screen.getAllByRole('menuitem')
832-
const itemLabels = menuitems.map(item => item.textContent)
855+
const itemLabels = menuitems
856+
.filter(item => !item.className.includes('disabled'))
857+
.map(item => item.textContent)
833858
expect(itemLabels).toStrictEqual(['Modify and Run', 'Modify and Queue'])
834859
})
835860

836-
it('should present the correct options for the main row with checkpoints', () => {
861+
it('should enable the correct options for the main row with checkpoints', () => {
837862
renderTableWithoutRunningExperiments()
838863

839864
const target = screen.getByText('main')
840865
fireEvent.contextMenu(target, { bubbles: true })
841866

842867
advanceTimersByTime(100)
843868
const menuitems = screen.getAllByRole('menuitem')
844-
const itemLabels = menuitems.map(item => item.textContent)
869+
const itemLabels = menuitems
870+
.filter(item => !item.className.includes('disabled'))
871+
.map(item => item.textContent)
845872
expect(itemLabels).toStrictEqual([
846873
'Modify and Run',
847874
'Modify and Resume',
@@ -850,15 +877,17 @@ describe('App', () => {
850877
])
851878
})
852879

853-
it('should present the correct option for an experiment with checkpoints and close on esc', () => {
880+
it('should enable the correct option for an experiment with checkpoints and close on esc', () => {
854881
renderTableWithoutRunningExperiments()
855882

856883
const target = screen.getByText('[exp-e7a67]')
857884
fireEvent.contextMenu(target, { bubbles: true })
858885

859886
advanceTimersByTime(100)
860887
const menuitems = screen.getAllByRole('menuitem')
861-
const itemLabels = menuitems.map(item => item.textContent)
888+
const itemLabels = menuitems
889+
.filter(item => !item.className.includes('disabled'))
890+
.map(item => item.textContent)
862891
expect(itemLabels).toStrictEqual([
863892
'Show Logs',
864893
'Apply to Workspace',
@@ -876,6 +905,24 @@ describe('App', () => {
876905
expect(screen.queryAllByRole('menuitem')).toHaveLength(0)
877906
})
878907

908+
it('should not close when a disabled item is clicked', () => {
909+
renderTableWithoutRunningExperiments()
910+
911+
const target = screen.getByText('main')
912+
fireEvent.contextMenu(target, { bubbles: true })
913+
914+
advanceTimersByTime(100)
915+
916+
const disabledMenuItem = screen
917+
.getAllByRole('menuitem')
918+
.find(item => item.className.includes('disabled'))
919+
920+
expect(disabledMenuItem).toBeDefined()
921+
922+
disabledMenuItem && fireEvent.click(disabledMenuItem, { bubbles: true })
923+
expect(screen.queryAllByRole('menuitem')).toHaveLength(10)
924+
})
925+
879926
it('should be removed with a left click', () => {
880927
renderTableWithoutRunningExperiments()
881928

@@ -885,9 +932,12 @@ describe('App', () => {
885932
advanceTimersByTime(100)
886933
expect(screen.getAllByRole('menuitem')).toHaveLength(10)
887934

888-
fireEvent.click(window, { bubbles: true })
935+
fireEvent.click(row, {
936+
bubbles: true
937+
})
889938
advanceTimersByTime(100)
890-
expect(screen.queryAllByRole('menuitem')).toHaveLength(0)
939+
940+
expect(screen.queryByRole('menuitem')).not.toBeInTheDocument()
891941
})
892942

893943
it('should be removed with a left click on a different row', () => {
@@ -902,7 +952,7 @@ describe('App', () => {
902952
const commit = getRow('main')
903953
fireEvent.click(commit, { bubbles: true })
904954
advanceTimersByTime(100)
905-
expect(screen.queryAllByRole('menuitem')).toHaveLength(0)
955+
expect(screen.queryByRole('menuitem')).not.toBeInTheDocument()
906956
})
907957

908958
it('should be moved with a right click on the same row (not toggle)', () => {
@@ -921,19 +971,21 @@ describe('App', () => {
921971
expect(screen.queryAllByRole('menuitem')).toHaveLength(10)
922972
})
923973

924-
it('should present the Remove experiment option for the checkpoint tips', () => {
974+
it('should enable the Remove experiment option for the checkpoint tips', () => {
925975
renderTableWithoutRunningExperiments()
926976

927977
const target = screen.getByText('4fb124a')
928978
fireEvent.contextMenu(target, { bubbles: true })
929979

930980
advanceTimersByTime(100)
931981
const menuitems = screen.getAllByRole('menuitem')
932-
const itemLabels = menuitems.map(item => item.textContent)
982+
const itemLabels = menuitems
983+
.filter(item => !item.className.includes('disabled'))
984+
.map(item => item.textContent)
933985
expect(itemLabels).toContain('Remove')
934986
})
935987

936-
it('should present the remove option if only experiments are selected', () => {
988+
it('should enable the remove option if only experiments are selected', () => {
937989
renderTableWithoutRunningExperiments()
938990

939991
clickRowCheckbox('4fb124a')
@@ -944,7 +996,9 @@ describe('App', () => {
944996

945997
advanceTimersByTime(100)
946998
const menuitems = screen.getAllByRole('menuitem')
947-
const itemLabels = menuitems.map(item => item.textContent)
999+
const itemLabels = menuitems
1000+
.filter(item => !item.className.includes('disabled'))
1001+
.map(item => item.textContent)
9481002
expect(itemLabels).toContain('Remove Selected')
9491003

9501004
const removeOption = menuitems.find(item =>
@@ -961,7 +1015,7 @@ describe('App', () => {
9611015
})
9621016
})
9631017

964-
it('should present the push option if only experiments are selected', () => {
1018+
it('should enable the push option if only experiments are selected', () => {
9651019
renderTableWithoutRunningExperiments()
9661020

9671021
clickRowCheckbox('4fb124a')
@@ -972,7 +1026,9 @@ describe('App', () => {
9721026

9731027
advanceTimersByTime(100)
9741028
const menuitems = screen.getAllByRole('menuitem')
975-
const itemLabels = menuitems.map(item => item.textContent)
1029+
const itemLabels = menuitems
1030+
.filter(item => !item.className.includes('disabled'))
1031+
.map(item => item.textContent)
9761032
expect(itemLabels).toContain('Push Selected')
9771033

9781034
const pushOption = menuitems.find(item =>
@@ -989,7 +1045,7 @@ describe('App', () => {
9891045
})
9901046
})
9911047

992-
it('should present the stop option if only running experiments are selected', () => {
1048+
it('should enable the stop option if only running experiments are selected', () => {
9931049
renderTable()
9941050

9951051
clickRowCheckbox('4fb124a')
@@ -999,7 +1055,9 @@ describe('App', () => {
9991055

10001056
advanceTimersByTime(100)
10011057
const menuitems = screen.getAllByRole('menuitem')
1002-
const itemLabels = menuitems.map(item => item.textContent)
1058+
const itemLabels = menuitems
1059+
.filter(item => !item.className.includes('disabled'))
1060+
.map(item => item.textContent)
10031061
expect(itemLabels).toContain('Stop')
10041062

10051063
const stopOption = menuitems.find(item =>
@@ -1024,7 +1082,9 @@ describe('App', () => {
10241082

10251083
advanceTimersByTime(100)
10261084
const menuitems = screen.getAllByRole('menuitem')
1027-
const itemLabels = menuitems.map(item => item.textContent)
1085+
const itemLabels = menuitems
1086+
.filter(item => !item.className.includes('disabled'))
1087+
.map(item => item.textContent)
10281088
expect(itemLabels).toContain('Stop')
10291089

10301090
const stopOption = menuitems.find(item =>
@@ -1051,7 +1111,9 @@ describe('App', () => {
10511111

10521112
advanceTimersByTime(100)
10531113
const menuitems = screen.getAllByRole('menuitem')
1054-
const itemLabels = menuitems.map(item => item.textContent)
1114+
const itemLabels = menuitems
1115+
.filter(item => !item.className.includes('disabled'))
1116+
.map(item => item.textContent)
10551117
expect(itemLabels).toContain('Push')
10561118

10571119
const shareOption = menuitems.find(item =>
@@ -1076,11 +1138,13 @@ describe('App', () => {
10761138

10771139
advanceTimersByTime(100)
10781140
const menuitems = screen.getAllByRole('menuitem')
1079-
const itemLabels = menuitems.map(item => item.textContent)
1141+
const itemLabels = menuitems
1142+
.filter(item => !item.className.includes('disabled'))
1143+
.map(item => item.textContent)
10801144
expect(itemLabels).not.toContain('Push')
10811145
})
10821146

1083-
it('should always present the Plots options if multiple rows are selected', () => {
1147+
it('should always enable the Plots options if multiple rows are selected', () => {
10841148
renderTableWithoutRunningExperiments()
10851149

10861150
clickRowCheckbox('4fb124a')
@@ -1091,7 +1155,9 @@ describe('App', () => {
10911155

10921156
advanceTimersByTime(100)
10931157
const menuitems = screen.getAllByRole('menuitem')
1094-
const itemLabels = menuitems.map(item => item.textContent)
1158+
const itemLabels = menuitems
1159+
.filter(item => !item.className.includes('disabled'))
1160+
.map(item => item.textContent)
10951161
expect(itemLabels).toContain('Plot and Show')
10961162
expect(itemLabels).toContain('Plot')
10971163

@@ -1122,7 +1188,9 @@ describe('App', () => {
11221188

11231189
advanceTimersByTime(100)
11241190
const menuitems = screen.getAllByRole('menuitem')
1125-
const itemLabels = menuitems.map(item => item.textContent)
1191+
const itemLabels = menuitems
1192+
.filter(item => !item.className.includes('disabled'))
1193+
.map(item => item.textContent)
11261194
expect(itemLabels).toContain('Star')
11271195
})
11281196

‎webview/src/experiments/components/table/body/ExperimentGroup.tsx‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,12 @@ import { RowProp } from '../../../util/interfaces'
55

66
export const ExperimentGroup: React.FC<RowProp & BatchSelectionProp> = ({
77
row,
8-
contextMenuDisabled,
98
projectHasCheckpoints,
109
hasRunningExperiment,
1110
batchRowSelection
1211
}) => (
1312
<NestedRow
1413
row={row}
15-
contextMenuDisabled={contextMenuDisabled}
1614
projectHasCheckpoints={projectHasCheckpoints}
1715
hasRunningExperiment={hasRunningExperiment}
1816
batchRowSelection={batchRowSelection}

‎webview/src/experiments/components/table/body/NestedRow.tsx‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import { RowProp } from '../../../util/interfaces'
55

66
export const NestedRow: React.FC<RowProp & BatchSelectionProp> = ({
77
row,
8-
contextMenuDisabled,
98
projectHasCheckpoints,
109
hasRunningExperiment,
1110
batchRowSelection
@@ -14,7 +13,6 @@ export const NestedRow: React.FC<RowProp & BatchSelectionProp> = ({
1413
<RowContent
1514
row={row}
1615
className={styles.nestedRow}
17-
contextMenuDisabled={contextMenuDisabled}
1816
projectHasCheckpoints={projectHasCheckpoints}
1917
hasRunningExperiment={hasRunningExperiment}
2018
batchRowSelection={batchRowSelection}

‎webview/src/experiments/components/table/body/Row.tsx‎

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import cx from 'classnames'
2-
import React, { useCallback, useContext, useMemo, useState } from 'react'
2+
import React, { useCallback, useContext, useMemo } from 'react'
33
import { useSelector } from 'react-redux'
44
import { EXPERIMENT_WORKSPACE_ID } from 'dvc/src/cli/dvc/contract'
55
import { isQueued, isRunning } from 'dvc/src/experiments/webview/contract'
@@ -22,7 +22,6 @@ export const RowContent: React.FC<
2222
> = ({
2323
row,
2424
className,
25-
contextMenuDisabled,
2625
projectHasCheckpoints,
2726
hasRunningExperiment,
2827
batchRowSelection
@@ -68,22 +67,13 @@ export const RowContent: React.FC<
6867
}
6968
}, [subRows, selectedRows])
7069

71-
const [menuActive, setMenuActive] = useState<boolean>(false)
72-
7370
const running = isRunning(status)
7471
const queued = isQueued(status)
7572
const unselected = selected === false
7673
const isOdd = index % 2 !== 0 && !isRowSelected
7774

7875
return (
7976
<ContextMenu
80-
disabled={contextMenuDisabled}
81-
onShow={() => {
82-
setMenuActive(true)
83-
}}
84-
onHide={() => {
85-
setMenuActive(false)
86-
}}
8777
content={
8878
<RowContextMenu
8979
row={row}
@@ -107,8 +97,7 @@ export const RowContent: React.FC<
10797
[styles.evenRow]: !isOdd,
10898
[styles.workspaceRow]: isWorkspace,
10999
[styles.normalRow]: !isWorkspace,
110-
[styles.rowSelected]: isRowSelected,
111-
[styles.rowFocused]: menuActive
100+
[styles.rowSelected]: isRowSelected
112101
}
113102
)}
114103
tabIndex={0}

0 commit comments

Comments
 (0)