-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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.
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
[Dashboard] [Controls] Allow options list suggestions to be sorted #144867
Changes from 27 commits
a3404c0
18c48e4
0a322db
47cd768
b4ac643
193724b
17df990
c03103c
bd64b2e
1915044
b6959b6
1ad7414
4fb4744
6b7cf85
e2aa509
c2d3040
3940e9a
94a142d
21e869e
82fccc5
badf71c
169439f
e5b29f2
93f815b
cf28f3d
9398634
758466d
bf63f7c
717d720
81127f2
35ef455
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
import { Direction } from '@elastic/eui'; | ||
|
||
export type SortBy = '_count' | '_key'; | ||
|
||
export const DEFAULT_SORT: SortingType = { by: '_count', direction: 'desc' }; | ||
|
||
export const sortDirections: Readonly<Direction[]> = ['asc', 'desc'] as const; | ||
export type SortDirection = typeof sortDirections[number]; | ||
export interface SortingType { | ||
by: SortBy; | ||
direction: SortDirection; | ||
} | ||
|
||
export const getCompatibleSortingTypes = (type?: string): SortBy[] => { | ||
switch (type) { | ||
case 'ip': { | ||
return ['_count']; | ||
} | ||
default: { | ||
return ['_count', '_key']; | ||
} | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,43 +6,96 @@ | |
* Side Public License, v 1. | ||
*/ | ||
|
||
import React, { useState } from 'react'; | ||
import React, { useEffect, useMemo, useState } from 'react'; | ||
|
||
import { | ||
EuiFlexGroup, | ||
EuiFlexItem, | ||
EuiForm, | ||
EuiFormRow, | ||
EuiIconTip, | ||
EuiSuperSelectOption, | ||
EuiSpacer, | ||
EuiSuperSelect, | ||
EuiSwitch, | ||
EuiSwitchEvent, | ||
EuiButtonGroup, | ||
toSentenceCase, | ||
Direction, | ||
} from '@elastic/eui'; | ||
import { css } from '@emotion/react'; | ||
|
||
import { | ||
getCompatibleSortingTypes, | ||
sortDirections, | ||
DEFAULT_SORT, | ||
SortBy, | ||
} from '../../../common/options_list/suggestions_sorting'; | ||
import { OptionsListStrings } from './options_list_strings'; | ||
import { ControlEditorProps, OptionsListEmbeddableInput } from '../..'; | ||
|
||
interface OptionsListEditorState { | ||
singleSelect?: boolean; | ||
sortDirection: Direction; | ||
runPastTimeout?: boolean; | ||
singleSelect?: boolean; | ||
hideExclude?: boolean; | ||
hideExists?: boolean; | ||
hideSort?: boolean; | ||
sortBy: SortBy; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Not sure how other folks feel about this, but I like to preface most types with a little context - like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that's a good point! Since it's specific to options list (at least for now), I think it's a good idea to make the name reflect this 👍 |
||
} | ||
|
||
interface SwitchProps { | ||
checked: boolean; | ||
onChange: (event: EuiSwitchEvent) => void; | ||
} | ||
|
||
type SortItem = EuiSuperSelectOption<SortBy>; | ||
|
||
export const OptionsListEditorOptions = ({ | ||
initialInput, | ||
onChange, | ||
fieldType, | ||
}: ControlEditorProps<OptionsListEmbeddableInput>) => { | ||
const [state, setState] = useState<OptionsListEditorState>({ | ||
singleSelect: initialInput?.singleSelect, | ||
sortDirection: initialInput?.sort?.direction ?? DEFAULT_SORT.direction, | ||
sortBy: initialInput?.sort?.by ?? DEFAULT_SORT.by, | ||
runPastTimeout: initialInput?.runPastTimeout, | ||
singleSelect: initialInput?.singleSelect, | ||
hideExclude: initialInput?.hideExclude, | ||
hideExists: initialInput?.hideExists, | ||
hideSort: initialInput?.hideSort, | ||
}); | ||
|
||
useEffect(() => { | ||
// when field type changes, ensure that the selected sort type is still valid | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice. I did a lot of testing where I would select a sort, then change the field type looking for bugs before I saw this |
||
if (!getCompatibleSortingTypes(fieldType).includes(state.sortBy)) { | ||
onChange({ sort: DEFAULT_SORT }); | ||
setState((s) => ({ ...s, sortBy: DEFAULT_SORT.by, sortDirection: DEFAULT_SORT.direction })); | ||
} | ||
}, [fieldType, onChange, state.sortBy]); | ||
|
||
const sortByOptions: SortItem[] = useMemo(() => { | ||
return getCompatibleSortingTypes(fieldType).map((key: SortBy) => { | ||
return { | ||
value: key, | ||
inputDisplay: OptionsListStrings.editorAndPopover.sortBy[key].getSortByLabel(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Excellent use of the strings file! |
||
'data-test-subj': `optionsListEditor__sortBy_${key}`, | ||
}; | ||
}); | ||
}, [fieldType]); | ||
|
||
const sortOrderOptions = useMemo(() => { | ||
return sortDirections.map((key) => { | ||
return { | ||
id: key, | ||
value: key, | ||
iconType: `sort${toSentenceCase(key)}ending`, | ||
'data-test-subj': `optionsListEditor__sortOrder_${key}`, | ||
label: OptionsListStrings.editorAndPopover.sortOrder[key].getSortByLabel(), | ||
}; | ||
}); | ||
}, []); | ||
|
||
const SwitchWithTooltip = ({ | ||
switchProps, | ||
label, | ||
|
@@ -77,6 +130,7 @@ export const OptionsListEditorOptions = ({ | |
onChange({ singleSelect: !state.singleSelect }); | ||
setState((s) => ({ ...s, singleSelect: !s.singleSelect })); | ||
}} | ||
data-test-subj={'optionsListControl__allowMultipleAdditionalSetting'} | ||
/> | ||
</EuiFormRow> | ||
<EuiFormRow> | ||
|
@@ -88,6 +142,7 @@ export const OptionsListEditorOptions = ({ | |
setState((s) => ({ ...s, hideExclude: !s.hideExclude })); | ||
if (initialInput?.exclude) onChange({ exclude: false }); | ||
}} | ||
data-test-subj={'optionsListControl__hideExcludeAdditionalSetting'} | ||
/> | ||
</EuiFormRow> | ||
<EuiFormRow> | ||
|
@@ -102,8 +157,74 @@ export const OptionsListEditorOptions = ({ | |
if (initialInput?.existsSelected) onChange({ existsSelected: false }); | ||
}, | ||
}} | ||
data-test-subj={'optionsListControl__hideExistsAdditionalSetting'} | ||
/> | ||
</EuiFormRow> | ||
<EuiFormRow> | ||
<> | ||
<EuiSwitch | ||
label={OptionsListStrings.editor.getHideSortingTitle()} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @elastic/kibana-docs I would appreciate some help with the wording for the "Hide sort" setting and with the title of the additional form that shows up when this setting is set to So there are two pieces of copy to consider: (1) I think (1) is already pretty straightforward - if this setting is set to The wording of (2) is more difficult - basically, if the author hides the sorting button from the popover, then they most provide a default sorting method to use for this control since the analyst is no longer able to change it dynamically - hence, why the secondary form shows up when (1) is toggled to Update There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Heenawter Here are some suggestions: 1. Allow dynamic sorting of suggestions 2. Default sort order (Maybe you don't need to repeat the word "suggestions" because it is appears in the text for the toggle?) 3. Ignore sorting when “Show only selected” is true. 4. Select a sorting strategy > Define the sort order 5 What control does "Delete control" delete? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gchaps Thanks so much 🙇 I changed everything in 82fccc5, and I think things are much clearer now!
The GIF I included is of the control editor flyout - so, if you are editing an existing control, selecting the "Delete control" option will delete the control that you are currently editing 👍 |
||
checked={!state.hideSort} | ||
onChange={() => { | ||
onChange({ hideSort: !state.hideSort }); | ||
setState((s) => ({ ...s, hideSort: !s.hideSort })); | ||
}} | ||
data-test-subj={'optionsListControl__hideSortAdditionalSetting'} | ||
/> | ||
{state.hideSort && ( | ||
<EuiForm className="optionsList--hiddenEditorForm"> | ||
<> | ||
<EuiSpacer size="s" /> | ||
<EuiFormRow | ||
display={'rowCompressed'} | ||
label={OptionsListStrings.editor.getSuggestionsSortingTitle()} | ||
> | ||
<EuiButtonGroup | ||
buttonSize="compressed" | ||
options={sortOrderOptions} | ||
idSelected={state.sortDirection} | ||
onChange={(value) => { | ||
onChange({ | ||
sort: { | ||
direction: value as Direction, | ||
by: state.sortBy, | ||
}, | ||
}); | ||
setState((s) => ({ ...s, sortDirection: value as Direction })); | ||
}} | ||
legend={OptionsListStrings.editorAndPopover.getSortDirectionLegend()} | ||
/> | ||
</EuiFormRow> | ||
<EuiFormRow | ||
display={'rowCompressed'} | ||
css={css` | ||
margin-top: 8px !important; | ||
`} | ||
hasEmptyLabelSpace={false} | ||
> | ||
<EuiSuperSelect | ||
onChange={(value) => { | ||
onChange({ | ||
sort: { | ||
direction: state.sortDirection, | ||
by: value, | ||
}, | ||
}); | ||
setState((s) => ({ ...s, sortBy: value })); | ||
}} | ||
options={sortByOptions} | ||
valueOfSelected={state.sortBy} | ||
data-test-subj={'optionsListControl__chooseSortBy'} | ||
compressed={true} | ||
/> | ||
</EuiFormRow> | ||
|
||
<EuiSpacer size="s" /> | ||
</> | ||
</EuiForm> | ||
)} | ||
</> | ||
</EuiFormRow> | ||
<EuiFormRow> | ||
<SwitchWithTooltip | ||
label={OptionsListStrings.editor.getRunPastTimeoutTitle()} | ||
|
@@ -115,6 +236,7 @@ export const OptionsListEditorOptions = ({ | |
setState((s) => ({ ...s, runPastTimeout: !s.runPastTimeout })); | ||
}, | ||
}} | ||
data-test-subj={'optionsListControl__runPastTimeoutAdditionalSetting'} | ||
/> | ||
</EuiFormRow> | ||
</> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice addition here to make it more modular!
Because the default spread happens at the end, if we ever introduced something to the default state, like
availableOptions: []
for instance, it would overwrite the additional state in this test which could cause test failures. I would move this to the top so anything declared in these mocks overrides the default.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! Yeah, that's a good point. Fixed 👍