[Dashboard] [Controls] Allow options list suggestions to be sorted #144867

Heenawter · 2022-11-08T23:36:17Z

Closes #140174
Closes #145040
Closes #146086

Summary

This PR adds two features to the options list control:

A button in the options list popover that gives users the ability to change how the suggestions are sorted
A per-control setting that disables the ability to dynamically sort which, if set to false, presents the author with the ability to select one of the four sorting methods for that specific control to use

Design considerations

@elastic/kibana-design

As noted by Andrea when looking at the preliminary behaviour of this feature, the "Show only selected" toggle has increased in importance because of the new sorting mechanic - after all, when making selections and then changing the sort method, your selections can appear to be "lost" if you have enough unique values in the control's field.

In the original designs, the "Clear all selections" button was first in the popover's action bar - however, I found that I kept accidentally clicking this in my testing when switching between searching, sorting, making selections, changing sorting, showing only selected options, etc. etc. I found that the following design felt a lot more natural for the placement of the "Clear all selections" button:

Note that, once #143585 is resolved, this will no longer be as much of a concern because we will be moving, at the very least, the "Clear all selections" to be a floating action. That being said, this new order for the actions is, in my opinion, a good compromise in the mean time. Very much open to feedback, though!

Video

OptionsListSortingUpdate.mov

Testing Notes

There are a few things to consider when testing:

Does the dynamic sorting give you expected results when sorting various field types?
- Note that IP fields only support document count sorting, so ensure that "Alphabetical" sorting does not show up in the sorting list during either creation or as part of the popover sorting.
When setting the "Allow suggestions to be sorted" toggle to false, it should always default to "Document count (descending)" to prevent invalid sort selections. For example, consider the following:
- Create an options list control on some keyword field
- Set the sort to alphabetical (either ascending or descending) in the popover
- Edit that control and change it to an IP field
- Set "Allow suggestions to be sorted" to `false
- The default sort should be "Document count (descending)" and not "Alphabetical (descending/ascending)", since alphabetical sorting would be invalid in this case.

Flaky Test Runner

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

…-fix'

andreadelrio

@Heenawter Design changes LGTM! Just left one comment. Thanks for being open to changes at the last minute. Great job.

andreadelrio · 2022-11-22T23:22:34Z

src/plugins/controls/public/options_list/components/options_list_popover_sorting_button.tsx

+import { OptionsListStrings } from './options_list_strings';
+import { optionsListReducers } from '../options_list_reducers';
+
+const SORT_POPOVER_WIDTH = 220;


Might be better to use a Eui size variable to get to the 220 here. That way we remain consistent with the recommendation not to have hard coded values.

Good point 👍 Done in 9398634

ThomThomson

Tested this locally on Chrome and the sort looks great and works as expected! I was a little confused by the icons, (which is totally out of the scope of this PR) but aside from that everything looks and works great! Left some tiny nits, but really the code looks great too. Especially happy with the additions of functional and unit tests. Looks awesome.

LGTM!

ThomThomson · 2022-11-24T18:20:09Z

src/plugins/controls/common/options_list/mocks.tsx

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


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.

Oh! Yeah, that's a good point. Fixed 👍

ThomThomson · 2022-11-24T18:28:07Z

src/plugins/controls/public/options_list/components/options_list_editor_options.tsx

  hideExclude?: boolean;
  hideExists?: boolean;
+  hideSort?: boolean;
+  sortBy: SortBy;


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.

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 👍

ThomThomson · 2022-11-24T18:30:25Z

src/plugins/controls/public/options_list/components/options_list_editor_options.tsx

  });

+  useEffect(() => {
+    // when field type changes, ensure that the selected sort type is still valid


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

ThomThomson · 2022-11-24T18:31:00Z

src/plugins/controls/public/options_list/components/options_list_editor_options.tsx

+    return getCompatibleSortingTypes(fieldType).map((key: SortBy) => {
+      return {
+        value: key,
+        inputDisplay: OptionsListStrings.editorAndPopover.sortBy[key].getSortByLabel(),


Excellent use of the strings file!

ThomThomson · 2022-11-24T18:35:44Z

test/functional/apps/dashboard_elements/controls/options_list.ts

@@ -427,6 +507,8 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
          await dashboardControls.optionsListOpenPopover(controlId);
          await dashboardControls.optionsListPopoverSelectOption('B');
          await dashboardControls.optionsListEnsurePopoverIsClosed(controlId);
+          await dashboard.waitForRenderComplete();


Awesome! Just a reminder to merge from upstream and unskip this one!

kibana-ci · 2022-11-24T19:46:48Z

💚 Build Succeeded

Buildkite Build
Commit: 35ef455

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`controls`	157	159	+2

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`controls`	228	233	+5

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`controls`	475.4KB	478.0KB	+2.7KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`controls`	7	8	+1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`controls`	21.8KB	25.7KB	+3.9KB

Unknown metric groups

API count

id	before	after	diff
`controls`	237	242	+5

ESLint disabled in files

id	before	after	diff
`osquery`	1	2	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`fleet`	59	65	+6
`osquery`	109	115	+6
`securitySolution`	443	449	+6
total			+20

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	68	74	+6
`osquery`	110	117	+7
`securitySolution`	520	526	+6
total			+21

History

💚 Build #90441 succeeded 758466d
💔 Build #90392 failed 9398634
💚 Build #90160 succeeded cf28f3d
💔 Build #90135 failed 169439f
💔 Build #89880 failed 82fccc5
💚 Build #89720 succeeded 94a142d

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @Heenawter

* main: (30 commits) [Cloud Posture] test latest findings table sort (elastic#144668) [api-docs] 2022-11-28 Daily api_docs build (elastic#146359) [api-docs] 2022-11-27 Daily api_docs build (elastic#146353) [api-docs] 2022-11-26 Daily api_docs build (elastic#146350) [DataViews] Fix form validation UX when the same data view name already exists (elastic#146126) [Discover] Prevent agg based visualizations of Discover saved objects with adhoc data views (elastic#145583) [Health Gateway] Update response aggregation (elastic#145761) [api-docs] 2022-11-25 Daily api_docs build (elastic#146341) [Metric threshold rule] Adds new context variable for group by keys (elastic#145654) [Controls] [Portable Dashboards] Add control group renderer example plugin (elastic#146189) Refactor Observability Overview Page (elastic#146182) Send complete test data to xMatters, so it can create an alert (elastic#145431) [Dashboard] [Controls] Allow options list suggestions to be sorted (elastic#144867) Add open API specification for list connector types (elastic#145951) skip flaky suite (elastic#146086) [ML] Removing duplicate tooltip text (elastic#146308) Refactor Rules Page (elastic#146193) [DOCS] Alert limit for cases (elastic#145950) Extend session index fields mapping with a session creation timestamp. (elastic#145997) [Files] Move <Image /> component to `@kbn/shared-ux` package (elastic#145995) ...

…tions (#146241) Closes #145039 ## Summary This PR adds a badge beside each options list suggestion that displays how many document that value appears in. https://user-images.githubusercontent.com/8698078/209020139-e0f16eae-6643-432b-9c40-f1a52fc7adf4.mov Note that, in the above video, clicking `"Show only selected"` removes the badge regardless of the validity of the selection. As described in #144867 (comment) and the related discussion, it is theoretically possible to still show this badge, thus allowing sorting even when `"Show only selected"` is `on`. However, my investigation into this took me on the following journey: 1. Document count **should not** be stored as part of the `explicitInput`, so `selections` should remain a string array 2. To get around this, the document count could be stored as part of the `validSelections` instead, which is part of the component state. That is, `validSelections` would be an `OptionsListSuggestions` object rather than a string array 3. To make (2) happen, we would need to add a secondary API endpoint specifically for translating the `explicitInput` selections array to the `validSelections` object After realizing (3), it became obvious that adding sorting support to `"Show only selected"` was probably not worth the code complexity that is required - so, at least until we get requests for this feature, I opted to not include it as part of this PR. After all, `"Show only selected"` is ultimately meant to make it easier to deselect/manage your selections, so showing the document count + allowing sorting in this case doesn't have much of an impact. For the same reasons, searching is also disabled when `"Show only selected"` is `on`: <p align="center"><img src="https://user-images.githubusercontent.com/8698078/209021944-c57ea04f-1278-4b6d-ae65-88b2591ba9d6.gif"/></p> > **Note** > The above screenshots and videos are slightly out-of-date. Refer to #146241 (comment) to see how the design of the document count was changed to reduce visual clutter. ### Flaky Test Runner - `control_group_chaining.ts` <a href="https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1669"><img src="https://user-images.githubusercontent.com/8698078/209016909-cb2cebf4-b069-4270-977b-a64a320d398d.png"/></a> - `options_list.ts` <a href="https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1670"><img src="https://user-images.githubusercontent.com/8698078/209020916-f322bfdb-799d-4aab-9726-9f46e130e779.png"/></a> ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

Closes #146086 ## Summary Backports the flaky test fix from #144867 to `8.6` <a href="https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1720"><img src="https://user-images.githubusercontent.com/8698078/211655383-3fa1ef92-d402-4efe-9b51-23a4ea6727d9.png"/></a>

## Summary - #148331: [Updated screenshots](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html) - #146335: [Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#search-or-filter-your-data) - #146363: [Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#edit-panels) - #144867: [Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html#edit-controls)

## Summary - elastic#148331: [Updated screenshots](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html) - elastic#146335: [Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#search-or-filter-your-data) - elastic#146363: [Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#edit-panels) - elastic#144867: [Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html#edit-controls) (cherry picked from commit e57883f)

## Summary - elastic#148331: [Updated screenshots](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html) - elastic#146335: [Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#search-or-filter-your-data) - elastic#146363: [Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#edit-panels) - elastic#144867: [Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html#edit-controls)

## Summary - #148331: [Updated screenshots](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html) - #146335: [Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#search-or-filter-your-data) - #146363: [Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#edit-panels) - #144867: [Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html#edit-controls)

Heenawter self-assigned this Nov 8, 2022

Heenawter added loe:large Large Level of Effort v8.7.0 and removed loe:medium Medium Level of Effort v8.6.0 labels Nov 14, 2022

Heenawter force-pushed the add-options-list-sorting_2022-11-07 branch 17 times, most recently from 0367fe5 to 2dac2c2 Compare November 16, 2022 23:31

Heenawter and others added 3 commits November 22, 2022 14:38

Make sorting popover smaller

e5b29f2

Merge branch 'main' into add-options-list-sorting_2022-11-07

93f815b

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

cf28f3d

…-fix'

andreadelrio approved these changes Nov 22, 2022

View reviewed changes

Heenawter and others added 2 commits November 23, 2022 09:23

Fix styling to use EuiSize constants

9398634

Merge branch 'main' into add-options-list-sorting_2022-11-07

758466d

ThomThomson approved these changes Nov 24, 2022

View reviewed changes

Heenawter and others added 4 commits November 24, 2022 11:41

Address feedback

bf63f7c

Merge branch 'main' into add-options-list-sorting_2022-11-07

717d720

Unskip test

81127f2

Clean up options list sort order strings

35ef455

Heenawter enabled auto-merge (squash) November 24, 2022 19:12

Heenawter merged commit 1ed31e1 into elastic:main Nov 24, 2022

kibanamachine added the backport:skip This commit does not require backporting label Nov 24, 2022

nlatipov mentioned this pull request Dec 6, 2022

[Unified search] Sort the dataviews on the picker alphabetically #137247

Closed

Heenawter mentioned this pull request Dec 19, 2022

[Dashboard] [Controls] Show document count beside options list suggestions #146241

Merged

7 tasks

Heenawter deleted the add-options-list-sorting_2022-11-07 branch December 30, 2022 18:50

Heenawter mentioned this pull request Jan 10, 2023

[Dashboard] [Controls] Backport flaky test fix #148662

Merged

KOTungseth added a commit to KOTungseth/kibana that referenced this pull request Feb 21, 2023

Adds content for elastic#144867

d6040c0

KOTungseth mentioned this pull request Feb 21, 2023

[DOCS] 8.7 Presentation docs #151797

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dashboard] [Controls] Allow options list suggestions to be sorted #144867

[Dashboard] [Controls] Allow options list suggestions to be sorted #144867

Heenawter commented Nov 8, 2022 •

edited

andreadelrio left a comment

andreadelrio Nov 22, 2022

Heenawter Nov 23, 2022

ThomThomson left a comment

ThomThomson Nov 24, 2022

Heenawter Nov 24, 2022

ThomThomson Nov 24, 2022

Heenawter Nov 24, 2022

ThomThomson Nov 24, 2022

ThomThomson Nov 24, 2022

ThomThomson Nov 24, 2022

kibana-ci commented Nov 24, 2022

API count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

[Dashboard] [Controls] Allow options list suggestions to be sorted #144867

[Dashboard] [Controls] Allow options list suggestions to be sorted #144867

Conversation

Heenawter commented Nov 8, 2022 • edited

Summary

Design considerations

Video

Testing Notes

Checklist

For maintainers

andreadelrio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomThomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Nov 24, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

Heenawter commented Nov 8, 2022 •

edited