-
Notifications
You must be signed in to change notification settings - Fork 8.4k
[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
Conversation
5284cd1
to
e9d713c
Compare
64d61e3
to
39b626e
Compare
33acbc8
to
5312bb5
Compare
aba07da
to
27989ae
Compare
This reverts commit 28fb423.
I agree that using a |
There was a problem hiding this 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.
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.
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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great addition!! 👍🏻 🙌🏻
Thanks for the review @chandlerprall @nreese @mdefazio @rshen91 @MichaelMarcialis ! I made the change to use the I also updated the design to align with your suggestion Michael 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? |
There was a problem hiding this 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.
Thanks for making these edits!
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. |
Thanks for the review @mdefazio
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 @nreese: can you have another review on this? thanks! |
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VisEditors code LGTM 👌🏼
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
ESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @sebelga |
There was a problem hiding this 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!
There was a problem hiding this 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
There was a problem hiding this 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
* 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) ...
In this PR I've udpated the
<TableListView />
tag filtering mechanism. We can now filter by tagsTags can be included or excluded (by Ctrl + click).
Screenshots
How to test
Plugin updated
The following plugins have been updated
Changes made to to the Saved object tagging API
getTagList()
handler to the api to allow consumers to retrieve the list of available tags.tagRender()
handler to the<TagList />
props to be able to control how the tag is renderedparse_search_query
logic to check forincluded
andnot included
tags and return atagReferencesToExclude
along with thetagReferences
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