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

Heenawter · 2022-11-23T22:11:33Z

Summary

This PR adds a badge beside each options list suggestion that displays how many document that value appears in.

Screen.Recording.2022-12-21.at.4.05.46.PM.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:

Document count should not be stored as part of the explicitInput, so selections should remain a string array
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
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:

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
options_list.ts

Checklist

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)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
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

Heenawter · 2022-12-21T23:34:41Z

src/plugins/controls/public/options_list/components/options_list_popover.test.tsx

+    const validSelection = findTestSubject(popover, 'optionsList-control-selection-bark');
+    expect(validSelection.text()).toEqual('bark75');
+    const title = findTestSubject(popover, 'optionList__ignoredSelectionLabel').text();
+    expect(title).toEqual('Ignored selection');


Because I refactored the optionsListPopoverGetAvailableOptions, it no longer returns "Ignored selection" / "Ignored selections" as part of the selections array. So, I added these unit tests to ensure that we don't lose test coverage for the singular/plural cases of this label 👍

This is likely a very good change anyway! Nice cleanup.

Heenawter · 2022-12-21T23:35:03Z

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

+                aria-label={
+                  showOnlySelected
+                    ? OptionsListStrings.popover.getAllOptionsButtonTitle()
+                    : OptionsListStrings.popover.getSelectedOptionsButtonTitle()
+                }


I must have somehow grabbed the wrong aria-label when refactoring the popover to be multiple smaller components - woops! Fixed now :)

Heenawter · 2022-12-21T23:35:15Z

src/plugins/controls/public/options_list/components/options_list_strings.ts

-    getLoadingMessage: () =>
-      i18n.translate('controls.optionsList.popover.loading', {
-        defaultMessage: 'Loading options',


A bunch of these OptionsListStrings were not being used - they should all be cleaned up now.

Heenawter · 2022-12-21T23:36:02Z

src/plugins/controls/public/options_list/components/options_list_strings.ts

    getSortDisabledTooltip: () =>
      i18n.translate('controls.optionsList.popover.sortDisabledTooltip', {
-        defaultMessage: 'Ignore sorting when “Show only selected” is true.',
+        defaultMessage: 'Sorting is ignored when “Show only selected” is true',
+      }),


@elastic/kibana-docs Since this tooltip is sticking around as described in the description of this PR, I reworded this to be passive rather than active. To me, the previous wording made it seem like the user could somehow change this behaviour, but they can't - whenever they click "Show only selected", sorting is always ignored!

Interested in your thoughts on this change 👍

This change make sense to me.

Heenawter · 2022-12-21T23:36:19Z

src/plugins/controls/public/options_list/components/options_list_strings.ts

+    getDocumentCountTooltip: (documentCount: number) =>
+      i18n.translate('controls.optionsList.popover.documentCountTooltip', {
+        defaultMessage:
+          'This value appears in {documentCount} {documentCount, plural, one {document} other {documents}}',
+        values: { documentCount },


@elastic/kibana-docs This text is for the tooltip that shows up when hovering on the document count:

Let me know if you think this wording is clear 👍

Text LGTM. Our guideline is to end tooltips with a period. However, none of the tooltip text in this PR does that. For consistency within Lens, we can let it go now and make that change in one global sweep.

Heenawter · 2022-12-21T23:36:40Z

test/functional/page_objects/dashboard_page_controls.ts

@@ -375,11 +386,44 @@ export class DashboardPageControls extends FtrService {
    return +(await availableOptions.getAttribute('data-option-count'));
  }

-  public async optionsListPopoverGetAvailableOptions(filterOutExists: boolean = true) {
+  public async optionsListPopoverGetAvailableOptions() {


Note that the filterOutExists parameter is no longer necessary because the tests that used this were removed as part of removing the "Allow exists" toggle from the UI.

elasticmachine · 2022-12-21T23:54:50Z

Pinging @elastic/kibana-presentation (Team:Presentation)

andreadelrio · 2022-12-28T20:06:42Z

Seems like the count badges are not visible (or are partially displayed) when the values are really long.

Heenawter · 2022-12-30T17:08:51Z

@andreadelrio Oh, good catch!! Fixed in d6033b0

Not that this code should all be cleaned up once we can finally make the transition to using EuiSelectable with the append property for the badge, which is currently blocked until Kibana is bumped to EUI v72.0.0. Once this happens though, the truncation and everything else should be handled for us + a11y would be greatly improved, which would be awesome 👀

Heenawter · 2023-01-03T19:05:52Z

Design Update

After feedback from @andreadelrio, we decided that the badge took up a bit too much real estate and added a lot of visual clutter. To clean this up, we opted to move away from using EuiBadge and instead display the document count with some styled EuiText, like so:

As part of this, we considered how it would look when the suggestions text was extremely long. However, with the truncation added in d6033b0, it actually still stands out well enough even without a background colour:

…-ref HEAD~1..HEAD --fix'

ThomThomson

Ran this locally and looked through the code, left a few small nits, but nothing major! LGTM!

ThomThomson · 2023-01-03T21:15:28Z

src/plugins/controls/common/options_list/suggestions_sorting.ts

+  direction: 'desc',
+};
+
+export const optionsListSortDirections: Readonly<Direction[]> = ['asc', 'desc'] as const;


I'm wondering why you go through the trouble of creating a new type out of the EUI direction type like this when you could use the EUI type directly or just do a simple union like:

type OptionsListSortDirection = 'asc' | 'desc';

I see what you've done here, and how it works, but I'm not sure I see the benefit?

ThomThomson · 2023-01-03T21:17:24Z

src/plugins/controls/public/options_list/components/options_list_popover.test.tsx

+    const validSelection = findTestSubject(popover, 'optionsList-control-selection-bark');
+    expect(validSelection.text()).toEqual('bark75');
+    const title = findTestSubject(popover, 'optionList__ignoredSelectionLabel').text();
+    expect(title).toEqual('Ignored selection');


This is likely a very good change anyway! Nice cleanup.

ThomThomson · 2023-01-03T21:29:34Z

src/plugins/controls/public/options_list/options_list_reducers.ts

-    state: WritableDraft<OptionsListReduxState>,
-    action: PayloadAction<string[]>
-  ) => {
+  deselectOptions: (state: WritableDraft<OptionsListReduxState>, action: PayloadAction<string>) => {


Why have you changed the payload action type here from string[] to string?

Woops! That was probably leftover from playing around with storing the document count as part of the selections array 🙇 It didn't break anything cause it looks like we never actually use deselectOptions, we only ever use deselectOption.... Do you know why this reducer even exists, now that you mention it? 👀

Will change this back to string[] though, regardless

Hurm. If it's never used we should probably delete it. It might be a holdover from some earlier development.

Good call! Will do 👍

Heenawter · 2023-01-03T21:42:27Z

@ThomThomson

I'm wondering why you go through the trouble of creating a new type out of the EUI direction type like this when you could use the EUI type directly or just do a simple union like:
type OptionsListSortDirection = 'asc' | 'desc';
I see what you've done here, and how it works, but I'm not sure I see the benefit?

I did this so I could iterate through the values in .../options_list_popover_sorting_button.tsx like so:

 optionsListSortDirections.map((key) => {
        return {
          id: key,
          iconType: `sort${toSentenceCase(key)}ending`,
          'data-test-subj': `optionsList__sortOrder_${key}`,
          label: OptionsListStrings.editorAndPopover.sortOrder[key].getSortOrderLabel(),
        } as SortOrderItem;
      }),

I don't think I could do something like that if I just used a union type? If there's a way around this, though, I would be totally down 'cause I know the way I did it is a tad bit messy 👀

ThomThomson · 2023-01-03T21:50:55Z

Ah that makes sense. You can't loop that way through a union type unfortunately, because it doesn't exist at runtime.

I would still argue that since there are only two sort directions, and we don't anticipate ever adding more, that it's overkill to even loop to define the sort buttons. Wouldn't something like this work just as well?

const sortOrderItems: SortOrderItem[] = [
  {
        id: 'asc',
        iconType: `sortAscending`,
        'data-test-subj': `optionsList__sortOrder_asc`,
        label: OptionsListStrings.editorAndPopover.sortOrder.asc.getSortOrderLabel(),
    },
    {
        id: 'desc',
        iconType: `sortDescending`,
        'data-test-subj': `optionsList__sortOrder_desc`,
        label: OptionsListStrings.editorAndPopover.sortOrder.desc.getSortOrderLabel(),
    }
];

The downside being more lines of code, the upside being it doesn't require that typing funkiness, and it's more readable and obvious.

Heenawter · 2023-01-03T22:00:03Z

@ThomThomson Yeah, that's a good call 👍 I agree - cleaner + more readable is probably better than slightly shorter code in this case. Cleaned this up in 5ae9e80

kibana-ci · 2023-01-03T23:00:11Z

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id	before	after	diff
`controls`	455.4KB	456.2KB	+793.0B

Page load bundle

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

id	before	after	diff
`controls`	32.8KB	32.8KB	-34.0B

History

💚 Build #97950 succeeded 3c082bf
💚 Build #97866 succeeded 6a9967d
💔 Build #97574 failed 70bc212
💔 Build #97564 failed d6033b0
💔 Build #96465 failed 2eef7c8
💔 Build #96436 failed 78f01c051c82aab03d5a3d36660dae2321677645

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

cc @Heenawter

Heenawter self-assigned this Nov 23, 2022

Heenawter force-pushed the show-document-count-in-options-list_2022-11-23 branch 6 times, most recently from 7a2c1ae to b0436c6 Compare November 24, 2022 21:49

Heenawter force-pushed the show-document-count-in-options-list_2022-11-23 branch 12 times, most recently from 0d429e6 to ea520d0 Compare December 20, 2022 23:45

Heenawter force-pushed the show-document-count-in-options-list_2022-11-23 branch 2 times, most recently from d7cd0ad to 54719f8 Compare December 21, 2022 22:33

First implementation of storing doc count in explicit input

3b88fac

Heenawter commented Dec 21, 2022

View reviewed changes

Fix a11y issues

2eef7c8

Heenawter marked this pull request as ready for review December 21, 2022 23:54

Heenawter requested a review from a team as a code owner December 21, 2022 23:54

Heenawter and others added 2 commits December 30, 2022 09:21

Merge branch 'main' into show-document-count-in-options-list_2022-11-23

60b2ec2

Add truncation to suggestions so badge stays visible

d6033b0

Heenawter and others added 3 commits December 30, 2022 11:31

Merge branch 'main' into show-document-count-in-options-list_2022-11-23

70bc212

Merge branch 'main' into show-document-count-in-options-list_2022-11-23

6a9967d

Change styling of badge to text

6424e66

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

3c082bf

…-ref HEAD~1..HEAD --fix'

ThomThomson approved these changes Jan 3, 2023

View reviewed changes

Remove unused reducer

24fac49

Clean up sort typing

5ae9e80

Heenawter merged commit 37a6636 into elastic:main Jan 3, 2023

kibanamachine added the backport:skip This commit does not require backporting label Jan 3, 2023

Heenawter deleted the show-document-count-in-options-list_2022-11-23 branch January 3, 2023 23:05

nickpeihl mentioned this pull request Jan 23, 2024

[Controls] Reintroduce Author Config Settings #155074

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Heenawter commented Nov 23, 2022 •

edited

Loading

Heenawter Dec 21, 2022

ThomThomson Jan 3, 2023

Heenawter Dec 21, 2022

Heenawter Dec 21, 2022

Heenawter Dec 21, 2022

gchaps Dec 22, 2022

Heenawter Dec 21, 2022

gchaps Dec 22, 2022

Heenawter Dec 21, 2022

elasticmachine commented Dec 21, 2022

andreadelrio commented Dec 28, 2022

Heenawter commented Dec 30, 2022 •

edited

Loading

Heenawter commented Jan 3, 2023 •

edited

Loading

ThomThomson left a comment

ThomThomson Jan 3, 2023

ThomThomson Jan 3, 2023

ThomThomson Jan 3, 2023

Heenawter Jan 3, 2023

ThomThomson Jan 3, 2023

Heenawter Jan 3, 2023

Heenawter commented Jan 3, 2023

ThomThomson commented Jan 3, 2023

Heenawter commented Jan 3, 2023

kibana-ci commented Jan 3, 2023

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

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

Conversation

Heenawter commented Nov 23, 2022 • edited Loading

Summary

Flaky Test Runner

Checklist

For maintainers

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

Choose a reason for hiding this comment

elasticmachine commented Dec 21, 2022

andreadelrio commented Dec 28, 2022

Heenawter commented Dec 30, 2022 • edited Loading

Heenawter commented Jan 3, 2023 • edited Loading

Design Update

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

Heenawter commented Jan 3, 2023

ThomThomson commented Jan 3, 2023

Heenawter commented Jan 3, 2023

kibana-ci commented Jan 3, 2023

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

History

Heenawter commented Nov 23, 2022 •

edited

Loading

Heenawter commented Dec 30, 2022 •

edited

Loading

Heenawter commented Jan 3, 2023 •

edited

Loading