[Dashboard][Controls] Add Control Group Search Settings by ThomThomson · Pull Request #128090 · elastic/kibana

ThomThomson · 2022-03-18T18:33:44Z

Summary

This PR adds new settings to the control group, allowing configuration of:

Query settings: Query settings influence which pieces of state from the query bar are applied to the controls. Generally they should be all set to on or off, but there are advanced use cases where an author may want to ignore the timerange, filter pills, or the querry bar separately.

Validation: When Controls query for data, they also send a validation query to see if their selection would cover any documents. If the validation setting is off, these validation results are not used. If the setting is on, selections which are invalid are ignored.

Hierarchical chaining: With Hierarchical chining on, controls pass their filters from left to right. With Hierarchical chaining off, each control is independently filtered by the query bar settings, and are unrelated.

Tests

This PR also adds tests to ensure that each setting works properly. Additionally, the functional test suite has been split up into 4 files under a new folder in dashboard_elements

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

…d query bar sync to the Control Group

…lGroupSearchSettings

… group input

andreadelrio · 2022-03-21T19:29:15Z

@ThomThomson sent some suggestions for the flyout (PR). Mainly trying to make the three switches less prominent as right now they seem to stand out a lot more than the two fields above (Layout and Default width).

I would also like to suggest that we consider changing the wording of the Layout field to Label position: Inline, Above. I think this would make it more clear. We got some feedback from earlier usability testing about confusion around that field. I think if users see "Control settings > Layout > Double line" they might be tricked into thinking they are setting two rows of controls.

…bana into controls/controlGroupSearchSettings

ThomThomson · 2022-03-21T21:43:35Z

@andreadelrio design changes look great and have been merged into this PR. I also really appreciate the new copy idea for the label position setting. That makes it so much more clear.

I have implemented the new Copy, and am hoping that @KOTungseth likes it too when she does her copy review!

…Style settings to labelPosition

andreadelrio · 2022-03-21T22:36:35Z

@andreadelrio design changes look great and have been merged into this PR. I also really appreciate the new copy idea for the label position setting. That makes it so much more clear.

I have implemented the new Copy, and am hoping that @KOTungseth likes it too when she does her copy review!

@ThomThomson Awesome! to keep things consistent we should really rename Title to Label in the "Create control" form.

elasticmachine · 2022-03-23T13:19:31Z

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

jloleysens

AppServices changes LGTM!

Heenawter

~~I was able to get in a situation where I couldn't unselect an option via the following steps:~~

1. Create two controls - for example, I did lastName and gender for the E-Commerce sample data
2. Turn off hierarchical chaining
3. Make selections in both controls - for example, I chose gender = 'female' and lastName = 'McDonald'
4. Turn hierarchical chaining back on
5. In the situation above, McDonald was no longer a valid female selection so it doesn't appear in my list (can't even search it), but it's still showing as being selected

~~Once I unselect female with hierarchical chaining on, I once again have access to unselect McDonald - but this probably isn't ideal? Unsure how we should deal with that.~~

Ignore this 🙃 It was a matter of case sensitivity - I searched for mc and not Mc, which is why it wasn't showing up. Might be worth considering turning off case sensitivity in searches though?

Will continue to test and comment anything else I notice - so consider this a code review + a partial behavior review 👍

Heenawter · 2022-03-23T15:15:54Z

src/plugins/controls/common/control_group/types.ts

  width: ControlWidth;
 }

+export type ControlGroupChainingSystem = 'HIERARCHICAL' | 'NONE';


Just a general style question for my own future reference - why did you choose ALL CAPS for these strings? Other constant strings, such as the different control widths/styles are all camel case (auto, oneLine, etc.), so I'm just curious about what differentiates this one.

That's a good question! I chose all caps for these values because it was originally an enum, and I changed it to a union string type because they're better DX-wise.

I am not sure if we have a naming convention for union string types, but I have no strong feelings one way or another, and could quite easily change it! Let's figure out which casing is used more frequently and update accordingly.

src/plugins/controls/public/control_group/editor/control_group_editor.tsx

Heenawter · 2022-03-23T16:50:23Z

src/plugins/controls/public/control_group/editor/control_group_editor.tsx

-                  updateAllControlWidths(currentWidth);
-                }
+                applyChangesToInput();
                onClose();


For specifically the onClose action - now that we have more settings and users could, in theory, lose their selections by accident, do we want to have a warning when the user cancels/clicks out of the control settings overlay like we do for adding a control? I could honestly argue both sides - pop up warnings are annoying, but are they more annoying than losing your selections? 😆 @andreadelrio thoughts?

I am more on the side of a warning, as long as it only happens if the settings have changed from the initial settings. This feels to me like an enhancement we should log!

src/plugins/controls/public/control_group/embeddable/control_group_container.tsx

src/plugins/dashboard/common/embeddable/dashboard_container_persistable_state.ts

Heenawter

One bug I found when testing.
When I turn off hierarchical chaining, when there is more than one control, only the filter for the last control seems to be applied. It works fine if I just have a control on gender, for example - but as soon as I have gender and lastName, only the lastName control will actually be applied and the gender control is ignored. Here's a video of what I'm seeing:

2022-03-23_UnableToDeleteControls2.mp4

Heenawter

I have some confusion about what the Sync with query bar option does. Basically, assume I have a dashboard with the E-Commerce data where this option is fully turned off. I have a filter pill for gender = 'female' and I have a control for gender where male is selected. I assume this should show no data, but it doesn't? And this happens regardless of whether Validate user selections is true or not.

This is probably just my own misunderstanding, but some clarification would be appreciated 👍

ThomThomson · 2022-03-23T19:35:01Z

Thanks for the super-thorough review Hannah! I will respond to your questions here:

Case insensitivity - this unfortunately isn't as easy as a configuration setting. It's an enhancement we will need to track and log against the Options List control. Basically, there is some intersection between the type of the field, the field's analyzer, and the way that we query it which needs to be sorted out. The existing autocomplete functionality (in the filter pills and query bar) is case sensitive too, but I think we can figure out a good way to handle this specifically for the Options List.
Hierarchical chaining off bug - this is a fairly sizeable issue! I know exactly what's causing it and how to fix it, and hopefully will have this fixed by tomorrow. Thank you for finding this!
Sync with Query Bar: Basically with sync off, the controls should not be filtered by the dashboard query, time range, or filter pills. What you're running into here is the filter overwriting logic.
- When sync query and validate selections are on Filters that come earlier, like your filter pill narrow down available options in controls that come later
- Filters that come later in the chain override any earlier filters which are created using the same field.
- So in your example, with validate user selections and sync with query bar off, the filter pill with gender = ' female' is being overwritten with the control selection gender = 'male'. So only documents relating to male should be shown in the dashboard.

Heenawter · 2022-03-23T19:44:01Z

Case insensitivity - this unfortunately isn't as easy as a configuration setting. It's an enhancement we will need to track and log against the Options List control. Basically, there is some intersection between the type of the field, the field's analyzer, and the way that we query it which needs to be sorted out. The existing autocomplete functionality (in the filter pills and query bar) is case sensitive too, but I think we can figure out a good way to handle this specifically for the Options List.

Ahhh, that makes sense! Definitely think it's worth addressing this in the future, but seems like it's not worth it right away 👍

Hierarchical chaining off bug - this is a fairly sizeable issue! I know exactly what's causing it and how to fix it, and hopefully will have this fixed by tomorrow. Thank you for finding this!

Perfect!! 😄 I'll approve once that is all fixed - I played around quite a bit with the settings, and wasn't able to find any other blockers and/or bugs.

Sync with Query Bar: Basically with sync off, the controls should not be filtered by the dashboard query, time range, or filter pills. What you're running into here is the filter overwriting logic.

Gotchya, gotchya. That makes a lot more sense, thanks - I felt like I was going crazy, hah 😄

…lGroupSearchSettings

crob611

Looks good to me.

src/plugins/dashboard/common/embeddable/dashboard_control_group.ts

kibana-ci · 2022-03-23T22:21:50Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`controls`	143	145	+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`	124	127	+3
`embeddable`	384	385	+1
total			+4

Async chunks

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

id	before	after	diff
`controls`	399.3KB	403.4KB	+4.1KB
`dashboard`	294.8KB	294.8KB	+18.0B
total			+4.1KB

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
`dashboard`	15	14	-1
`embeddable`	5	4	-1
total			-2

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.4KB	23.4KB	+2.0KB
`dashboard`	66.5KB	66.7KB	+215.0B
total			+2.2KB

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/development-plugin-saved-objects.html#_mappings

id	before	after	diff
`dashboard`	20	22	+2

Unknown metric groups

API count

id	before	after	diff
`controls`	130	133	+3
`embeddable`	471	474	+3
total			+6

History

💚 Build #32890 succeeded 45581cd
💔 Build #32422 failed 3537e93
💔 Build #32362 failed 481083f
💔 Build #31949 failed 68208ad
💔 Build #31906 failed 895e959

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

Added ability to toggle hierarchical chaining, control validation, an…

895e959

…d query bar sync to the Control Group

ThomThomson and others added 4 commits March 18, 2022 17:55

Merge branch 'main' of github.com:elastic/kibana into controls/contro…

68208ad

…lGroupSearchSettings

Merge branch 'main' of github.com:elastic/kibana into controls/contro…

52cf79c

…lGroupSearchSettings

adjust flyout

66198ec

Fixed chainingSystem typo and fixed undefined keys overriding control…

481083f

… group input

ThomThomson added 2 commits March 21, 2022 17:09

type fix

2e53a5e

Merge branch 'controls-settings-design' of github.com:andreadelrio/ki…

766c44f

…bana into controls/controlGroupSearchSettings

added data-test-subjs which were removed accidentally, rename control…

3537e93

…Style settings to labelPosition

split functional tests, moved them to a folder in dasboard_elements.

45581cd

ThomThomson marked this pull request as ready for review March 23, 2022 13:19

ThomThomson requested review from a team as code owners March 23, 2022 13:19

jloleysens approved these changes Mar 23, 2022

View reviewed changes

botelastic bot added the Feature:Embedding Embedding content via iFrame label Mar 23, 2022

Heenawter reviewed Mar 23, 2022

View reviewed changes

ThomThomson mentioned this pull request Mar 23, 2022

[Controls] Range slider #125584

Merged

9 tasks

Heenawter reviewed Mar 23, 2022

View reviewed changes

ThomThomson added 2 commits March 23, 2022 16:16

Make onChildChange part of the chainingSystem

7e854fd

Merge branch 'main' of github.com:elastic/kibana into controls/contro…

25ed111

…lGroupSearchSettings

crob611 approved these changes Mar 23, 2022

View reviewed changes

src/plugins/dashboard/common/embeddable/dashboard_control_group.ts Outdated Show resolved Hide resolved

safe JSON parse

bc0fe54

ThomThomson merged commit 82d4cd5 into elastic:main Mar 23, 2022

ThomThomson added v8.2.0 backport:skip This PR does not require backporting and removed backport:skip This PR does not require backporting v8.3.0 labels Mar 24, 2022

Conversation

ThomThomson commented Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Tests

Checklist

Uh oh!

andreadelrio commented Mar 21, 2022

Uh oh!

ThomThomson commented Mar 21, 2022

Uh oh!

andreadelrio commented Mar 21, 2022

Uh oh!

elasticmachine commented Mar 23, 2022

Uh oh!

jloleysens left a comment

Choose a reason for hiding this comment

Uh oh!

Heenawter left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Heenawter Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

ThomThomson Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Heenawter Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ThomThomson Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Heenawter left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Heenawter left a comment

Choose a reason for hiding this comment

Uh oh!

ThomThomson commented Mar 23, 2022

Uh oh!

Heenawter commented Mar 23, 2022

Uh oh!

crob611 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibana-ci commented Mar 23, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

Saved Objects .kibana field count

API count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ThomThomson commented Mar 18, 2022 •

edited

Loading

Heenawter left a comment •

edited

Loading

Heenawter Mar 23, 2022 •

edited

Loading

Heenawter left a comment •

edited

Loading