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.

Sign up for GitHub

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

Jump to bottom

[Dashboard] [Controls] Add excludes toggle to options list #142780

Merged
merged 22 commits into from
Oct 25, 2022
Merged

[Dashboard] [Controls] Add excludes toggle to options list #142780

merged 22 commits into from
Oct 25, 2022

Conversation

Heenawter
Copy link
Contributor

@Heenawter Heenawter commented Oct 5, 2022 •
edited
Loading

Closes #140699
Closes #143671
Closes #136547
Closes #134369

Summary

This PR adds an include/exclude toggle to the popover of the options list so that users can exclude their selections rather than only allowing them to exclusively include their selections.

Screen.Recording.2022-10-19.at.9.43.32.AM.mov

As part of this, this PR introduces a custom diffing system for options list panels, which were previously only compared using deepEqual. This comparison was insufficient because:

This PR also introduces a setting so that authors can disable this new option on a per-control basis - so, if they decide that they don't want their users to be able to exclude the control, or if it doesn't make sense that a specific field can be excluded, then they have the option to hide it.

Furthermore, this PR tidies up some low-hanging fruit with respect to the options list control, such as changing the copy and adding a tooltip for the "Ignore timeout" setting, cleaning up the mobile view, etc. These are each described further in the comments below.

Notes about Design Changes

The design of the options list popover was modified from the initial Figma designs in the following ways:

  1. Because we still want to show the field's title rather than replacing it with NOT, I opted to simply prepend NOT to the selected options instead:
    image

  2. Since the bottom right corner is normally reserved for "Apply" or "Confirm" buttons, @alexmarhaba had concerns about the location of the Clear selections button in the Figma designs. Therefore, I opted to leave the Clear selections button in the same location that it was previously - note, however, that there are plans to move this to a floating action once [Controls] Use Trigger / Actions Framework for Hover #143585 is resolved.
    image

Flaky Test Runner

Checklist

For maintainers

@Heenawter Heenawter added Feature:Dashboard Dashboard related features Feature:Input Control Input controls visualization Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:large Large Level of Effort impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. release_note:feature Makes this part of the condensed release notes Project:Controls v8.6.0 labels Oct 5, 2022
@Heenawter Heenawter self-assigned this Oct 5, 2022
@Heenawter Heenawter force-pushed the add-excludes-to-options-list_2022-10-04 branch 4 times, most recently from 6ad1ae6 to 1eb48a0 Compare October 5, 2022 22:39
@Heenawter Heenawter added loe:medium Medium Level of Effort and removed loe:large Large Level of Effort labels Oct 5, 2022
@Heenawter Heenawter force-pushed the add-excludes-to-options-list_2022-10-04 branch 5 times, most recently from 205ca04 to 3d442d7 Compare October 5, 2022 23:03
@Heenawter Heenawter force-pushed the add-excludes-to-options-list_2022-10-04 branch 5 times, most recently from c25a1a7 to 982b734 Compare October 17, 2022 15:14
@ThomThomson ThomThomson linked an issue Oct 18, 2022 that may be closed by this pull request
[Controls] Add Include / Exclude functionality for Options List #140699
Closed
@Heenawter Heenawter force-pushed the add-excludes-to-options-list_2022-10-04 branch from faabab5 to aeb6d28 Compare October 24, 2022 16:16
@Heenawter Heenawter added the ui-copy Review of UI copy with docs team is recommended label Oct 24, 2022
ThomThomson
ThomThomson approved these changes Oct 24, 2022
View reviewed changes
Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Amazing work! Looked through the code and left a few nits. Also ran it locally and tested out the new functionality. Everything is working great!

Really nice test coverage, both some unit tests and functional tests!

LGTM

src/plugins/controls/common/control_group/control_group_panel_diff_system.ts Outdated
Comment on lines 27 to 29
export const ControlPanelDiffSystems: {
[key in ControlGroupDiffSystem]: DiffSystem;
} = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice usage of function registry type architecture! It's temporary, but it's set up like a microcosm of the actual embeddable diffing system, so it'll be super easy to migrate over, even if we end up needing diffing systems for multiple control types in the meantime.

Note to self: I was thinking about when to move this into the embeddable, and I think it makes the most sense to do so when we tackle Control Group as a Panel, so let's keep that in mind when we write up that issue.

src/plugins/controls/common/control_group/control_group_panel_diff_system.ts Show resolved Hide resolved
src/plugins/controls/common/control_group/control_group_panel_diff_system.ts Outdated Show resolved Hide resolved
src/plugins/controls/common/control_group/control_group_panel_diff_system.ts Outdated Show resolved Hide resolved
src/plugins/controls/common/control_group/types.ts Outdated Show resolved Hide resolved
src/plugins/controls/public/options_list/components/options_list_control.tsx Show resolved Hide resolved
src/plugins/controls/public/options_list/embeddable/options_list_embeddable.tsx Outdated Show resolved Hide resolved
@andreadelrio
Copy link
Contributor

andreadelrio commented Oct 24, 2022 •
edited
Loading

@Heenawter @ThomThomson I have a UX question. What do you think about showing the red "NOT" in the input if the user has clicked "Exclude" but hasn't selected any values yet? On the pro side I think it could be beneficial in helping users discover the feature. On the (-) side, if they close the popover without making any selections, having a "NOT" in the input would be weird/confusing.

@Heenawter
Copy link
Contributor Author

Heenawter commented Oct 24, 2022 •
edited
Loading

@andreadelrio Yeah, not sure about this... I agree it would be nice for discoverability, but having "NOT Any" as the input might be a bit confusing because this suggests that you are... excluding all values or something? i.e. "NOT Any" would make me expect a no-data state.

I think we could change the wording to make this more clear, but I'm stumped on what that would look like 😆

@Heenawter Heenawter closed this Oct 24, 2022
@Heenawter Heenawter reopened this Oct 24, 2022
@andreadelrio
Copy link
Contributor

@andreadelrio Yeah, not sure about this... I agree it would be nice for discoverability, but having "NOT Any" as the input might be a bit confusing because this suggests that you are... excluding all values or something? i.e. "NOT Any" would make me expect a no-data state.

I think we could change the wording to make this more clear, but I'm stumped on what that would look like 😆

Oh, I hadn't even thought about Any, that just makes everything more complex. After thinking about this some I think it's better to keep the behavior as is.

@Heenawter Heenawter enabled auto-merge (squash) October 24, 2022 20:16
@Heenawter Heenawter force-pushed the add-excludes-to-options-list_2022-10-04 branch from 971f7d9 to 9367369 Compare October 24, 2022 22:40
@Heenawter Heenawter disabled auto-merge October 24, 2022 22:43
@Heenawter Heenawter enabled auto-merge (squash) October 25, 2022 16:16
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
controls 215 216 +1

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 220 224 +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 492.3KB 493.2KB +963.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 18.4KB 20.6KB +2.2KB
Unknown metric groups

API count

id before after diff
controls 229 233 +4

History

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

cc @Heenawter

@Heenawter Heenawter merged commit 7dd7a74 into elastic:main Oct 25, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Oct 25, 2022
@Heenawter Heenawter added loe:large Large Level of Effort and removed loe:medium Medium Level of Effort labels Oct 25, 2022
@Heenawter Heenawter mentioned this pull request Nov 16, 2022
Closed
@KOTungseth KOTungseth mentioned this pull request Nov 16, 2022
Merged
KOTungseth added a commit that referenced this pull request Nov 28, 2022
@KOTungseth @gchaps
[DOCS] Adds the 8.6 Presentation docs (#145474)
9a26af6 
## Summary

Adds the docs for the following:

- #141824
Doc preview:
https://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/get-started.html

- #142780
Doc preview:
https://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html

- #143762
Doc preview:
https://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html

Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com>
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 28, 2022
@KOTungseth
[DOCS] Adds the 8.6 Presentation docs (elastic#145474)
a3cfcbd 
## Summary

Adds the docs for the following:

- elastic#141824
Doc preview:
https://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/get-started.html

- elastic#142780
Doc preview:
https://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html

- elastic#143762
Doc preview:
https://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html

Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com>
(cherry picked from commit 9a26af6)
kibanamachine added a commit that referenced this pull request Nov 28, 2022
@kibanamachine @KOTungseth
[8.6] [DOCS] Adds the 8.6 Presentation docs (#145474) (#146430)
dad6bff 
# Backport

This will backport the following commits from `main` to `8.6`:
- [[DOCS] Adds the 8.6 Presentation docs
(#145474)](#145474)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Kaarina
Tungseth","email":"kaarina.tungseth@elastic.co"},"sourceCommit":{"committedDate":"2022-11-28T16:20:38Z","message":"[DOCS]
Adds the 8.6 Presentation docs (#145474)\n\n## Summary\r\n\r\nAdds the
docs for the following:\r\n\r\n- #141824\r\nDoc
preview:\r\nhttps://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/get-started.html\r\n\r\n-
#142780\r\nDoc
preview:\r\nhttps://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html\r\n\r\n-
#143762\r\nDoc
preview:\r\nhttps://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html\r\n\r\nCo-authored-by:
gchaps
<33642766+gchaps@users.noreply.github.com>","sha":"9a26af6facafa9519ac624901805a1a65c8abb09","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Docs","Team:Presentation","release_note:skip","v8.6.0","v8.7.0"],"number":145474,"url":"https://github.com/elastic/kibana/pull/145474","mergeCommit":{"message":"[DOCS]
Adds the 8.6 Presentation docs (#145474)\n\n## Summary\r\n\r\nAdds the
docs for the following:\r\n\r\n- #141824\r\nDoc
preview:\r\nhttps://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/get-started.html\r\n\r\n-
#142780\r\nDoc
preview:\r\nhttps://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html\r\n\r\n-
#143762\r\nDoc
preview:\r\nhttps://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html\r\n\r\nCo-authored-by:
gchaps
<33642766+gchaps@users.noreply.github.com>","sha":"9a26af6facafa9519ac624901805a1a65c8abb09"}},"sourceBranch":"main","suggestedTargetBranches":["8.6"],"targetPullRequestStates":[{"branch":"8.6","label":"v8.6.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/145474","number":145474,"mergeCommit":{"message":"[DOCS]
Adds the 8.6 Presentation docs (#145474)\n\n## Summary\r\n\r\nAdds the
docs for the following:\r\n\r\n- #141824\r\nDoc
preview:\r\nhttps://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/get-started.html\r\n\r\n-
#142780\r\nDoc
preview:\r\nhttps://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html\r\n\r\n-
#143762\r\nDoc
preview:\r\nhttps://kibana_145474.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html\r\n\r\nCo-authored-by:
gchaps
<33642766+gchaps@users.noreply.github.com>","sha":"9a26af6facafa9519ac624901805a1a65c8abb09"}}]}]
BACKPORT-->

Co-authored-by: Kaarina Tungseth <kaarina.tungseth@elastic.co>
@Heenawter Heenawter mentioned this pull request Jan 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KOTungseth KOTungseth KOTungseth left review comments

@andreadelrio andreadelrio andreadelrio left review comments

@ThomThomson ThomThomson ThomThomson approved these changes

Assignees

@Heenawter Heenawter

Labels
backport:skip This commit does not require backporting Feature:Dashboard Dashboard related features Feature:Input Control Input controls visualization impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:large Large Level of Effort Project:Controls release_note:feature Makes this part of the condensed release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas ui-copy Review of UI copy with docs team is recommended v8.6.0
Projects
None yet
Milestone
No milestone
7 participants
@Heenawter @elasticmachine @andreadelrio @kibana-ci @ThomThomson @KOTungseth @kibanamachine