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.

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

Merged
merged 31 commits into from Nov 24, 2022
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
a3404c0
Add non-functional sorting component to popover
Heenawter Nov 8, 2022
18c48e4
First version of sorting
Heenawter Nov 8, 2022
0a322db
Switch icon order
Heenawter Nov 14, 2022
47cd768
Move sort to explicit input rather than component state
Heenawter Nov 14, 2022
b4ac643
Clean up unnecessary debounce subject
Heenawter Nov 14, 2022
193724b
Auto close popover on selection
Heenawter Nov 14, 2022
17df990
Disable sorting when `show only selected`
Heenawter Nov 14, 2022
c03103c
Fix Jest unit tests
Heenawter Nov 15, 2022
bd64b2e
Add `i18n` support
Heenawter Nov 15, 2022
1915044
Switch property to string instead of object
Heenawter Nov 15, 2022
b6959b6
Add functional + unit tests
Heenawter Nov 15, 2022
1ad7414
Clean up code + fix flakiness of functional tests
Heenawter Nov 16, 2022
4fb4744
Add toggle to disable sorting
Heenawter Nov 16, 2022
6b7cf85
Clean up code and add more functional + unit tests
Heenawter Nov 17, 2022
e2aa509
Hide alphabetical sort for IP fields + add corresponding unit tests
Heenawter Nov 17, 2022
c2d3040
Fix flaky test
Heenawter Nov 18, 2022
3940e9a
Add different tooltip when "Show only selected" is `true`
Heenawter Nov 18, 2022
94a142d
Merge branch 'main' into add-options-list-sorting_2022-11-07
Heenawter Nov 21, 2022
21e869e
Change design of sort menu in popover + editor
Heenawter Nov 22, 2022
82fccc5
Change wording based on feedback
Heenawter Nov 22, 2022
badf71c
Fix unit + functional tests
Heenawter Nov 22, 2022
169439f
Clean up code
Heenawter Nov 22, 2022
e5b29f2
Make sorting popover smaller
Heenawter Nov 22, 2022
93f815b
Merge branch 'main' into add-options-list-sorting_2022-11-07
Heenawter Nov 22, 2022
cf28f3d
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine Nov 22, 2022
9398634
Fix styling to use EuiSize constants
Heenawter Nov 23, 2022
758466d
Merge branch 'main' into add-options-list-sorting_2022-11-07
Heenawter Nov 23, 2022
bf63f7c
Address feedback
Heenawter Nov 24, 2022
717d720
Merge branch 'main' into add-options-list-sorting_2022-11-07
Heenawter Nov 24, 2022
81127f2
Unskip test
Heenawter Nov 24, 2022
35ef455
Clean up options list sort order strings
Heenawter Nov 24, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -8,6 +8,7 @@

import deepEqual from 'fast-deep-equal';
import { omit, isEqual } from 'lodash';
import { DEFAULT_SORT } from '../options_list/suggestions_sorting';
import { OptionsListEmbeddableInput, OPTIONS_LIST_CONTROL } from '../options_list/types';

import { ControlPanelState } from './types';
Expand All @@ -32,7 +33,9 @@ export const ControlPanelDiffSystems: {
}

const {
sort: sortA,
exclude: excludeA,
hideSort: hideSortA,
hideExists: hideExistsA,
hideExclude: hideExcludeA,
selectedOptions: selectedA,
Expand All @@ -42,7 +45,9 @@ export const ControlPanelDiffSystems: {
...inputA
}: Partial<OptionsListEmbeddableInput> = initialInput.explicitInput;
const {
sort: sortB,
exclude: excludeB,
hideSort: hideSortB,
hideExists: hideExistsB,
hideExclude: hideExcludeB,
selectedOptions: selectedB,
Expand All @@ -54,11 +59,13 @@ export const ControlPanelDiffSystems: {

return (
Boolean(excludeA) === Boolean(excludeB) &&
Boolean(hideSortA) === Boolean(hideSortB) &&
Boolean(hideExistsA) === Boolean(hideExistsB) &&
Boolean(hideExcludeA) === Boolean(hideExcludeB) &&
Boolean(singleSelectA) === Boolean(singleSelectB) &&
Boolean(existsSelectedA) === Boolean(existsSelectedB) &&
Boolean(runPastTimeoutA) === Boolean(runPastTimeoutB) &&
deepEqual(sortA ?? DEFAULT_SORT, sortB ?? DEFAULT_SORT) &&
isEqual(selectedA ?? [], selectedB ?? []) &&
deepEqual(inputA, inputB)
);
Expand Down
7 changes: 5 additions & 2 deletions src/plugins/controls/common/options_list/mocks.tsx
Expand Up @@ -10,7 +10,10 @@ import { createReduxEmbeddableTools } from '@kbn/presentation-util-plugin/public

import { OptionsListEmbeddable, OptionsListEmbeddableFactory } from '../../public';
import { OptionsListComponentState, OptionsListReduxState } from '../../public/options_list/types';
import { optionsListReducers } from '../../public/options_list/options_list_reducers';
import {
getDefaultComponentState,
optionsListReducers,
} from '../../public/options_list/options_list_reducers';
import { ControlFactory, ControlOutput } from '../../public/types';
import { OptionsListEmbeddableInput } from './types';

Expand All @@ -20,7 +23,7 @@ const mockOptionsListComponentState = {
availableOptions: ['woof', 'bark', 'meow', 'quack', 'moo'],
invalidSelections: [],
validSelections: [],
searchString: { value: '', valid: true },
...getDefaultComponentState(),
Copy link
Contributor

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!

Super tiny nit that doesn't make a difference now but might later

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.

Copy link
Contributor Author

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 👍

} as OptionsListComponentState;

const mockOptionsListEmbeddableInput = {
Expand Down
31 changes: 31 additions & 0 deletions src/plugins/controls/common/options_list/suggestions_sorting.ts
@@ -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'];
}
}
};
6 changes: 5 additions & 1 deletion src/plugins/controls/common/options_list/types.ts
Expand Up @@ -6,9 +6,10 @@
* Side Public License, v 1.
*/

import type { Filter, Query, BoolQuery, TimeRange } from '@kbn/es-query';
import { FieldSpec, DataView, RuntimeFieldSpec } from '@kbn/data-views-plugin/common';
import type { Filter, Query, BoolQuery, TimeRange } from '@kbn/es-query';

import { SortingType } from './suggestions_sorting';
import { DataControlInput } from '../types';

export const OPTIONS_LIST_CONTROL = 'optionsListControl';
Expand All @@ -20,6 +21,8 @@ export interface OptionsListEmbeddableInput extends DataControlInput {
singleSelect?: boolean;
hideExclude?: boolean;
hideExists?: boolean;
hideSort?: boolean;
sort?: SortingType;
exclude?: boolean;
}

Expand Down Expand Up @@ -65,5 +68,6 @@ export interface OptionsListRequestBody {
textFieldName?: string;
searchString?: string;
fieldSpec?: FieldSpec;
sort?: SortingType;
fieldName: string;
}
Expand Up @@ -243,34 +243,41 @@ export const ControlEditor = ({
/>
</EuiFormRow>
<EuiFormRow label={ControlGroupStrings.manageControl.getWidthInputTitle()}>
<EuiButtonGroup
color="primary"
legend={ControlGroupStrings.management.controlWidth.getWidthSwitchLegend()}
options={CONTROL_WIDTH_OPTIONS}
idSelected={currentWidth}
onChange={(newWidth: string) => {
setCurrentWidth(newWidth as ControlWidth);
updateWidth(newWidth as ControlWidth);
}}
/>
</EuiFormRow>
{updateGrow ? (
<EuiFormRow>
<EuiSwitch
label={ControlGroupStrings.manageControl.getGrowSwitchTitle()}
<>
<EuiButtonGroup
color="primary"
checked={currentGrow}
onChange={() => {
setCurrentGrow(!currentGrow);
updateGrow(!currentGrow);
legend={ControlGroupStrings.management.controlWidth.getWidthSwitchLegend()}
options={CONTROL_WIDTH_OPTIONS}
idSelected={currentWidth}
onChange={(newWidth: string) => {
setCurrentWidth(newWidth as ControlWidth);
updateWidth(newWidth as ControlWidth);
}}
data-test-subj="control-editor-grow-switch"
/>
</EuiFormRow>
) : null}
{updateGrow && (
<>
<EuiSpacer size="s" />
<EuiSwitch
label={ControlGroupStrings.manageControl.getGrowSwitchTitle()}
color="primary"
checked={currentGrow}
onChange={() => {
setCurrentGrow(!currentGrow);
updateGrow(!currentGrow);
}}
data-test-subj="control-editor-grow-switch"
/>
</>
)}
</>
</EuiFormRow>
{CustomSettings && (factory as IEditableControlFactory).controlEditorOptionsComponent && (
<EuiFormRow label={ControlGroupStrings.manageControl.getControlSettingsTitle()}>
<CustomSettings onChange={onTypeEditorChange} initialInput={embeddable?.getInput()} />
<CustomSettings
onChange={onTypeEditorChange}
initialInput={embeddable?.getInput()}
fieldType={fieldRegistry[selectedField].field.type}
/>
</EuiFormRow>
)}
{removeControl && (
Expand Down
Expand Up @@ -87,3 +87,7 @@
.optionsList--filterGroup {
width: 100%;
}

.optionsList--hiddenEditorForm {
margin-left: $euiSizeXXL + $euiSizeM;
}
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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 OptionsListSortBy for instance. It makes the names longer which can be annoying to type, but that way we never confuse our system types for more generic / fundamental ones offered by other teams.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 useEffect. Very clean! 🔥

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(),
Copy link
Contributor

Choose a reason for hiding this comment

The 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,
Expand Down Expand Up @@ -77,6 +130,7 @@ export const OptionsListEditorOptions = ({
onChange({ singleSelect: !state.singleSelect });
setState((s) => ({ ...s, singleSelect: !s.singleSelect }));
}}
data-test-subj={'optionsListControl__allowMultipleAdditionalSetting'}
/>
</EuiFormRow>
<EuiFormRow>
Expand All @@ -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>
Expand All @@ -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()}
Copy link
Contributor Author

@Heenawter Heenawter Nov 18, 2022

Choose a reason for hiding this comment

The 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 false 🙇 Here's a quick GIF of this in action:

So there are two pieces of copy to consider: (1) "Allow suggestions to be sorted" and (2) "Suggestions sorting"

I think (1) is already pretty straightforward - if this setting is set to false, this hides the sorting menu from the control popover. Perhaps "Allow suggestions to be dynamically sorted" might be more specific? 🤷

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 false. I'm not sure if "Default suggestion sorting" or something along the lines of "Sort suggestions via..." would reflect this better? Definitely need some help with this one

Update
Also need some help with the text in the following tooltip: #144867 (comment)

Copy link
Contributor

@gchaps gchaps Nov 21, 2022

Choose a reason for hiding this comment

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

@Heenawter Here are some suggestions:

1.

Allow dynamic sorting of suggestions
Allow 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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!

What control does "Delete control" delete?

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()}
Expand All @@ -115,6 +236,7 @@ export const OptionsListEditorOptions = ({
setState((s) => ({ ...s, runPastTimeout: !s.runPastTimeout }));
},
}}
data-test-subj={'optionsListControl__runPastTimeoutAdditionalSetting'}
/>
</EuiFormRow>
</>
Expand Down