From 47e1860e342b3f83e1c0b65f10d287badc09c4e2 Mon Sep 17 00:00:00 2001 From: "Grot (@grafanabot)" <43478413+grafanabot@users.noreply.github.com> Date: Thu, 9 Mar 2023 12:21:16 +0000 Subject: [PATCH] [v9.4.x] OptionsPicker: Correctly highlight template variable value when filtering (#64394) OptionsPicker: Correctly highlight template variable value when filtering (#63495) * VariablePicker: Correctly highlight items when filtering * Change interactions for selecting values in variable options picker * Review (cherry picked from commit e4d591fc0161ed42d703b46052540eb5ddbe4a86) Co-authored-by: Dominik Prokop --- .../pickers/OptionsPicker/OptionsPicker.tsx | 1 - .../pickers/OptionsPicker/actions.test.ts | 57 +++++++++++++------ .../pickers/OptionsPicker/actions.ts | 7 ++- .../pickers/OptionsPicker/reducer.test.ts | 14 ++--- .../pickers/OptionsPicker/reducer.ts | 6 +- .../pickers/shared/VariableInput.tsx | 6 +- 6 files changed, 58 insertions(+), 33 deletions(-) diff --git a/public/app/features/variables/pickers/OptionsPicker/OptionsPicker.tsx b/public/app/features/variables/pickers/OptionsPicker/OptionsPicker.tsx index 0a93e7ca525..942cdedd0e6 100644 --- a/public/app/features/variables/pickers/OptionsPicker/OptionsPicker.tsx +++ b/public/app/features/variables/pickers/OptionsPicker/OptionsPicker.tsx @@ -150,7 +150,6 @@ export const optionPickerFactory = { .whenActionIsDispatched(navigateOptions('key', NavigationKey.moveDown, clearOthers)) .whenAsyncActionIsDispatched(navigateOptions('key', key, clearOthers), true); - const option = createOption(['sameLabel'], ['B'], true); - // Check selecting the second option triggers variables to update tester.thenDispatchedActionsShouldEqual( - toKeyedAction('key', toggleOption({ option: options[1], forceSelect: true, clearOthers })), - toKeyedAction('key', setCurrentVariableValue(toVariablePayload(variable, { option }))), - toKeyedAction('key', changeVariableProp(toVariablePayload(variable, { propName: 'queryValue', propValue: '' }))), - toKeyedAction('key', hideOptions()), - toKeyedAction('key', setCurrentVariableValue(toVariablePayload(variable, { option }))) + toKeyedAction('key', toggleOption({ option: options[1], forceSelect: false, clearOthers })) ); - expect(locationService.partial).toHaveBeenLastCalledWith({ 'var-Constant': ['B'] }); }); describe('when navigateOptions is dispatched with navigation key selectAndClose after highlighting the second option', () => { - it('then correct actions are dispatched', async () => { + it('then correct actions are dispatched for multi-value variable', async () => { const options = [createOption('A'), createOption('B'), createOption('C')]; const variable = createMultiVariable({ options, current: createOption(['A'], ['A'], true), includeAll: false }); const clearOthers = false; const key = NavigationKey.selectAndClose; + const tester = await reduxTester() + .givenRootReducer(getRootReducer()) + .whenActionIsDispatched( + toKeyedAction('key', addVariable(toVariablePayload(variable, { global: false, index: 0, model: variable }))) + ) + .whenActionIsDispatched(toKeyedAction('key', showOptions(variable))) + .whenActionIsDispatched(navigateOptions('key', NavigationKey.moveDown, clearOthers)) + .whenActionIsDispatched(navigateOptions('key', NavigationKey.moveDown, clearOthers)) + .whenActionIsDispatched(navigateOptions('key', NavigationKey.moveDown, clearOthers)) + .whenActionIsDispatched(navigateOptions('key', NavigationKey.moveUp, clearOthers)) + .whenAsyncActionIsDispatched(navigateOptions('key', key, clearOthers), true); + + tester.thenDispatchedActionsShouldEqual( + toKeyedAction('key', toggleOption({ option: options[1], forceSelect: false, clearOthers })) + ); + }); + + it('then correct actions are dispatched for single-value variable', async () => { + const options = [createOption('A'), createOption('B'), createOption('C')]; + const variable = createVariable({ options, current: createOption('A', 'A', true), includeAll: false }); + + const clearOthers = false; + const key = NavigationKey.selectAndClose; + const tester = await reduxTester() .givenRootReducer(getRootReducer()) .whenActionIsDispatched( @@ -246,9 +263,9 @@ describe('options picker actions', () => { .whenAsyncActionIsDispatched(navigateOptions('key', key, clearOthers), true); const option = { - ...createOption(['B']), + ...createOption('B'), selected: true, - value: ['B'], + value: 'B', }; tester.thenDispatchedActionsShouldEqual( @@ -261,7 +278,7 @@ describe('options picker actions', () => { toKeyedAction('key', hideOptions()), toKeyedAction('key', setCurrentVariableValue(toVariablePayload(variable, { option }))) ); - expect(locationService.partial).toHaveBeenLastCalledWith({ 'var-Constant': ['B'] }); + expect(locationService.partial).toHaveBeenLastCalledWith({ 'var-Constant': 'B' }); }); }); @@ -526,7 +543,7 @@ describe('options picker actions', () => { .whenActionIsDispatched(toggleOptionByHighlight('key', clearOthers)); const optionA = createOption('A'); - const optionBC = createOption('BC'); + const optionBD = createOption('BD'); tester.thenDispatchedActionsShouldEqual( toKeyedAction('key', toggleOption({ option: optionA, forceSelect: false, clearOthers })), @@ -534,13 +551,21 @@ describe('options picker actions', () => { toKeyedAction('key', updateOptionsAndFilter(variable.options)), toKeyedAction('key', moveOptionsHighlight(1)), toKeyedAction('key', moveOptionsHighlight(1)), - toKeyedAction('key', toggleOption({ option: optionBC, forceSelect: false, clearOthers })) + toKeyedAction('key', toggleOption({ option: optionBD, forceSelect: false, clearOthers })) ); }); }); }); function createMultiVariable(extend?: Partial): QueryVariableModel { + return createVariable({ + multi: true, + includeAll: true, + ...(extend ?? {}), + }); +} + +function createVariable(extend?: Partial): QueryVariableModel { return { ...initialVariableModelState, type: 'query', @@ -556,8 +581,8 @@ function createMultiVariable(extend?: Partial): QueryVariabl sort: VariableSort.alphabeticalAsc, refresh: VariableRefresh.never, regex: '', - multi: true, - includeAll: true, + multi: false, + includeAll: false, ...(extend ?? {}), }; } diff --git a/public/app/features/variables/pickers/OptionsPicker/actions.ts b/public/app/features/variables/pickers/OptionsPicker/actions.ts index 675adcf9376..a4c5b3bd134 100644 --- a/public/app/features/variables/pickers/OptionsPicker/actions.ts +++ b/public/app/features/variables/pickers/OptionsPicker/actions.ts @@ -34,8 +34,13 @@ export const navigateOptions = (rootStateKey: string, key: NavigationKey, clearO } if (key === NavigationKey.selectAndClose) { + const picker = getVariablesState(rootStateKey, getState()).optionsPicker; + + if (picker.multi) { + return dispatch(toggleOptionByHighlight(rootStateKey, clearOthers)); + } dispatch(toggleOptionByHighlight(rootStateKey, clearOthers, true)); - return await dispatch(commitChangesToVariable(rootStateKey)); + return dispatch(commitChangesToVariable(rootStateKey)); } if (key === NavigationKey.moveDown) { diff --git a/public/app/features/variables/pickers/OptionsPicker/reducer.test.ts b/public/app/features/variables/pickers/OptionsPicker/reducer.test.ts index e4da8a26d04..f6ccf40632e 100644 --- a/public/app/features/variables/pickers/OptionsPicker/reducer.test.ts +++ b/public/app/features/variables/pickers/OptionsPicker/reducer.test.ts @@ -535,7 +535,7 @@ describe('optionsPickerReducer', () => { ], selectedValues: [{ text: 'All', value: '$__all', selected: true }], queryValue: 'A', - highlightIndex: -1, + highlightIndex: 0, }); }); @@ -559,7 +559,7 @@ describe('optionsPickerReducer', () => { options: [{ text: 'All', value: '$__all', selected: true }], selectedValues: [{ text: 'All', value: '$__all', selected: true }], queryValue: 'A', - highlightIndex: -1, + highlightIndex: 0, }); }); }); @@ -583,7 +583,7 @@ describe('optionsPickerReducer', () => { options: [{ text: 'option:1337', value: 'option:1337', selected: false }], selectedValues: [], queryValue: 'option:1337', - highlightIndex: -1, + highlightIndex: 0, }); }); }); @@ -635,7 +635,7 @@ describe('optionsPickerReducer', () => { ], selectedValues: [{ text: 'B', value: 'B', selected: true }], queryValue: 'A', - highlightIndex: -1, + highlightIndex: 0, }) .whenActionIsDispatched(updateSearchQuery('')) .thenStateShouldEqual({ @@ -646,7 +646,7 @@ describe('optionsPickerReducer', () => { ], selectedValues: [{ text: 'B', value: 'B', selected: true }], queryValue: '', - highlightIndex: -1, + highlightIndex: 0, }) .whenActionIsDispatched(updateOptionsAndFilter(options)) .thenStateShouldEqual({ @@ -658,7 +658,7 @@ describe('optionsPickerReducer', () => { ], selectedValues: [{ text: 'B', value: 'B', selected: true }], queryValue: '', - highlightIndex: -1, + highlightIndex: 0, }); }); }); @@ -766,7 +766,7 @@ describe('optionsPickerReducer', () => { options: [{ text: 'option:11256', value: 'option:11256', selected: false }], selectedValues: [], queryValue: 'option:11256', - highlightIndex: -1, + highlightIndex: 0, }); }); }); diff --git a/public/app/features/variables/pickers/OptionsPicker/reducer.ts b/public/app/features/variables/pickers/OptionsPicker/reducer.ts index 535a0580d11..4d0d32d4f20 100644 --- a/public/app/features/variables/pickers/OptionsPicker/reducer.ts +++ b/public/app/features/variables/pickers/OptionsPicker/reducer.ts @@ -1,5 +1,5 @@ import { createSlice, PayloadAction } from '@reduxjs/toolkit'; -import { cloneDeep, isString, trim } from 'lodash'; +import { cloneDeep, isString, trimStart } from 'lodash'; import { applyStateChanges } from '../../../../core/utils/applyStateChanges'; import { ALL_VARIABLE_VALUE } from '../../constants'; @@ -197,7 +197,7 @@ const optionsPickerSlice = createSlice({ return state; }, updateOptionsAndFilter: (state, action: PayloadAction): OptionsPickerState => { - const searchQuery = trim((state.queryValue ?? '').toLowerCase()); + const searchQuery = trimStart((state.queryValue ?? '').toLowerCase()); state.options = action.payload.filter((option) => { const optionsText = option.text ?? ''; @@ -205,7 +205,7 @@ const optionsPickerSlice = createSlice({ return text.toLowerCase().indexOf(searchQuery) !== -1; }); - state.highlightIndex = -1; + state.highlightIndex = 0; return applyStateChanges(state, updateDefaultSelection, updateOptions); }, diff --git a/public/app/features/variables/pickers/shared/VariableInput.tsx b/public/app/features/variables/pickers/shared/VariableInput.tsx index 24117896e17..6040a20cb3b 100644 --- a/public/app/features/variables/pickers/shared/VariableInput.tsx +++ b/public/app/features/variables/pickers/shared/VariableInput.tsx @@ -8,15 +8,11 @@ export interface Props extends Omit, 'onChange onChange: (value: string) => void; onNavigate: (key: NavigationKey, clearOthers: boolean) => void; value: string | null; - currenthighlightindex?: number; } export class VariableInput extends PureComponent { onKeyDown = (event: React.KeyboardEvent) => { - if ( - NavigationKey[event.keyCode] && - !(event.keyCode === NavigationKey.select && this.props.currenthighlightindex === -1) - ) { + if (NavigationKey[event.keyCode] && event.keyCode !== NavigationKey.select) { const clearOthers = event.ctrlKey || event.metaKey || event.shiftKey; this.props.onNavigate(event.keyCode as NavigationKey, clearOthers); event.preventDefault();