Skip to content

[TableListView] Enhance tag filtering #142108

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

Merged
merged 49 commits into from
Nov 14, 2022

Conversation

sebelga
Copy link
Contributor

@sebelga sebelga commented Sep 28, 2022

In this PR I've udpated the <TableListView /> tag filtering mechanism. We can now filter by tags

  • from the search bar filter dropdown
  • directly from the table

Tags can be included or excluded (by Ctrl + click).

Screenshots

Screenshot 2022-11-08 at 14 24 30

Screenshot 2022-11-08 at 14 24 47

Screenshot 2022-11-08 at 14 25 01

How to test

  • Navigate to the Dashboard / Visualisation library / Maps listing page
  • Add some tags to the saved object if needed
  • Filter by tag the table by
    • Clicking the tag in the table or in the filter dropdown
    • Ctrl + click the tag in the table or in the filter dropdown
  • Open the filter drop down and make sure the "Manage all tags" redirect correctly to the Tag management page

Plugin updated

The following plugins have been updated

  • Dashboard
  • Visualisation
  • Maps

Changes made to to the Saved object tagging API

  • Added getTagList() handler to the api to allow consumers to retrieve the list of available tags.
  • Added tagRender() handler to the <TagList /> props to be able to control how the tag is rendered
  • Updated the parse_search_query logic to check for included and not included tags and return a tagReferencesToExclude along with the tagReferences array.

Release notes

The dashboard, visualize library and maps listing pages have been updated with an enhanced tag filtering. The new filtering allows tags to be either included or excluded from the search.

Fixes #135237

@sebelga sebelga force-pushed the table-list-view/tag-selector branch from 5284cd1 to e9d713c Compare October 3, 2022 15:25
@sebelga sebelga force-pushed the table-list-view/tag-selector branch 4 times, most recently from 64d61e3 to 39b626e Compare October 18, 2022 14:50
@sebelga sebelga force-pushed the table-list-view/tag-selector branch 2 times, most recently from 33acbc8 to 5312bb5 Compare October 24, 2022 08:31
@sebelga sebelga force-pushed the table-list-view/tag-selector branch 2 times, most recently from aba07da to 27989ae Compare November 8, 2022 09:32
@MichaelMarcialis
Copy link
Contributor

Thanks for sharing. It seems that they read the browser OS. Before taking any decision I'd like the input from the design team on this. cc @mdefazio @MichaelMarcialis

I agree that using a Control modifier across all operating systems for the exclusion filter action feels odd and that better alignment with other patterns across Kibana would be most ideal. Based on what @markov00 indicated above, it seems that show/hide and enabled/disabled related actions are currently being triggered with Shift + Click/Enter. With that in mind, I think that an exclusion filter action would make sense to be triggered with Command/Control (determined by OS detection) + Click/Enter.

Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

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

@sebelga Thanks for the ping!
Just had a few thoughts on the popover. I agree with @MichaelMarcialis 's comment about the modifier.

image

Changes:

  • Move 'Clear selection' to the header and change to our primary color and no icon (and adds the title 'Tags' to provide the space). I don't think this needs to be as ominous or frightening as it is currently.
  • Add a search bar
  • Keep the popover open while making selections
  • Adds a 'Save' button to the footer (for the reason in the previous bullet)
  • Updates the footer of the popover
  • Note that I updated the copy for 'Ctrl + click to filter out tags' to 'Ctrl + click to exclude'. We use 'Exclude' in the search bar, so I'd recommend doing the same here. Also removing the word 'tag' here—don't think it's necessary and if we want to reuse this elsewhere, we don't have to worry about updating that text to match.
    image

@chandlerprall
Copy link
Contributor

Yes I think we should align. It seems that if elastic charts already has a convention in place we could follow it here.

In my personal experience, shift seems to be more commonly used as the way to access the next layer of modifier shortcuts after the first set (ctrl, win/cmd, alt/option) are defined. On Windows, ctrl is more common as the first modifier, but on Mac it seems to be option. (nb: on mac, ctrl acts like the alternative mouse button was used)

And of course there are exceptions everywhere, and at this point I always have to read the tooltips or documentation to know which way an app implemented it. I've brought it up with the accessibility group and we'll take a deeper look and make a recommendation.

All that said, standardization across our products is the most important piece here.

@nreese
Copy link
Contributor

nreese commented Nov 9, 2022

In Maps, we use control key (or meta key on Mac) for keydown to enable scroll to zoom when embedding a map in a scrollable container like dashboard.

#135330

Copy link
Contributor

@rshen91 rshen91 left a comment

Choose a reason for hiding this comment

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

Great addition!! 👍🏻 🙌🏻

@sebelga
Copy link
Contributor Author

sebelga commented Nov 10, 2022

Thanks for the review @chandlerprall @nreese @mdefazio @rshen91 @MichaelMarcialis !

I made the change to use the command key in mac and the ctrl key in Windows.

I also updated the design to align with your suggestion Michael

Screenshot 2022-11-10 at 17 08 41

Regarding the search I am applying the same rule as for other search bar filter in EUI. To display the search box there need to be at least 10 items in the list. I am not sure there is much value to have a search box for 3 items. Thoughts?

Screenshot 2022-11-10 at 16 00 35

Screenshot 2022-11-10 at 16 01 02

@sebelga sebelga requested a review from mdefazio November 10, 2022 17:40
Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

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

Regarding the search I am applying the same rule as for other search bar filter in EUI. To display the search box there need to be at least 10 items in the list. I am not sure there is much value to have a search box for 3 items. Thoughts?

Yep, makes sense!

The only tiny nit that I found was when using the keyboard, and I select a tag from the list, I lose focus on the tag list and have to navigate back into it. Same for when I search for one as well.

I think i'd expect my focus to remain in the list after I've selected a tag so I can easily select another.

Kapture 2022-11-10 at 16 12 47

Thanks for making these edits!

@1Copenut
Copy link
Contributor

I've been researching this PR today and started a discussion in our Kibana Global Experience group: https://github.com/orgs/elastic/teams/kibana-global-experience

I think we have an excellent opportunity to rally around one pattern for these types of UI going forward. I will be discussing this further with Zuhair next week. His domain expertise in screen reader and keyboard UX will be extremely helpful.

@sebelga sebelga requested a review from a team as a code owner November 14, 2022 10:44
@sebelga
Copy link
Contributor Author

sebelga commented Nov 14, 2022

Thanks for the review @mdefazio

The only tiny nit that I found was when using the keyboard, and I select a tag from the list, I lose focus on the tag list and have to navigate back into it. Same for when I search for one as well.

I tried to fix it but the way the component cycle works with the EUI search bar filters makes it too hacky for a real solution. The problem is that every time the tag selection changes the component unmounts and remounts. I tried a hacky solution to force focus but then it is a poor experience for the users using a mouse.

We can revisit the way EUI allows custom_component to be passed as search filter to avoid the unmount/mount problem.

@nreese: can you have another review on this? thanks!

Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

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

Thanks @sebelga for the explanation and working through the other changes.

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

VisEditors code LGTM 👌🏼

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dashboard 340 345 +5
graph 249 254 +5
maps 958 963 +5
savedObjectsTagging 88 89 +1
visualizations 321 326 +5
total +21

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
savedObjectsTaggingOss 48 50 +2

Async chunks

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

id before after diff
dashboard 388.1KB 396.6KB +8.5KB
graph 441.8KB 450.2KB +8.4KB
maps 2.7MB 2.7MB +8.5KB
visualizations 255.2KB 263.6KB +8.4KB
total +33.8KB

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
@kbn/content-management-table-list 4 5 +1

Page load bundle

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

id before after diff
savedObjectsTagging 18.2KB 18.6KB +417.0B
visualizations 53.7KB 53.7KB +19.0B
total +436.0B
Unknown metric groups

API count

id before after diff
savedObjectsTaggingOss 94 98 +4

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 108 113 +5
securitySolution 441 447 +6
total +19

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 67 73 +6
osquery 109 115 +6
securitySolution 518 524 +6
total +20

History

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

cc @sebelga

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.

Dashboard Saved Object service changes LGTM!

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

LGTM, just changes of types in a single graph file, didn't test

@sebelga sebelga enabled auto-merge (squash) November 14, 2022 17:15
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

kibana-gis changes LGTM
code review

@sebelga sebelga merged commit a67776b into elastic:main Nov 14, 2022
@kibanamachine kibanamachine added v8.6.0 backport:skip This commit does not require backporting labels Nov 14, 2022
@sebelga sebelga deleted the table-list-view/tag-selector branch November 15, 2022 08:59
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 15, 2022
* main: (65 commits)
  Migrate server-side `Root` and `Server` to packages (elastic#144990)
  [Discover] Handle no data views state for `esQuery` alert (elastic#145052)
  [ML] Allow updates for number of allocations and priority for trained model deployments (elastic#144704)
  [api-docs] 2022-11-15 Daily api_docs build (elastic#145203)
  [Security solution] remove guided onboarding feature flag (elastic#144247)
  [DOCS] Automate final case APIs (elastic#145007)
  [Enterprise Search] Name and description flyout for connectors (elastic#143827)
  [Guided onboarding] Update header button logic (elastic#144634)
  [Lens] Multi metric partition charts (elastic#143966)
  [Dashboard] [Controls] Add unmapped runtime field support to options list (elastic#144947)
  [Security Solution] Add Task Metric Collection to New Tasks (elastic#145181)
  [TriggersActionsUi] disable jest config in CI (elastic#145186)
  [TableListView] Enhance tag filtering (elastic#142108)
  [Cloud Posture] Compliance by CIS section table (elastic#145114)
  [8.6][Session View] Fix hidden alert flyout  in session view (elastic#145141)
  [customIntegrations] async load all components (elastic#145166)
  Fix time for logs smoke tests in integration test (elastic#145130)
  [RAM] Update rule status (elastic#140882)
  Update babel (main) (elastic#145060)
  [Actionable Observability] Add context.alertDetailsUrl variable to action connector template for APM rule types (elastic#144791)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Content Management User generated content (saved objects) management release_note:enhancement Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TableListView] Custom filter panel for tags