-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[TSVB] Replace indexpattern input text with a combobox #82696
Conversation
@elasticmachine merge upstream |
1 similar comment
@elasticmachine merge upstream |
merge conflict between base and head |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
Hey @alexwizp , Curious as to why you chose the EuiSuggest component instead of the EuiComboBox? EuiSuggest is usually when the input is a compound of strings forming a "formula" with auto suggestions based on input location. Whereas this functionality is simply a multi-select. |
@cchaos please see #80849 (comment). In a nutshell, we cannot parse the current index pattern string to correctly cover all cases. |
@alexwizp it would be great if we could also use the keyboard here. I can only select index patterns with the mouse click. Moreover it partially solves the ux problem we are trying to solve here. User should remember a part of the index pattern. If we use the combobox with not multiselect? What kind of problems do we have? Can you remind me? |
@elasticmachine merge upstream |
@stratoula elastic/eui#4345 - reported issue for issues related to keyboard navigation @chaos let me show you 2 cases why we cannot use a multi-select component:
|
@elasticmachine merge upstream |
@cchaos, could you please help us find a better component for this feature or approve this approach |
b53f301
to
e7a8625
Compare
@alexwizp why don't we use the combobox |
|
I was thinking: If the user wants to select only one index pattern then she could use the combobox, otherwise she should use the free text of the combobox. |
@stratoula @ryankeairns one more case why I prefer to use Next case based on our sample data ( Screen.Recording.2021-01-15.at.11.57.35.AM.movIn Screen.Recording.2021-01-15.at.12.03.41.PM.mov |
@alexwizp thanx for this. He can't edit it but can he write it? Is it allowed? |
@stratoula yes, but if I understand our goal properly we are trying to simplify selecting indexes for our users. And for me it looks more like an autocomplete functionality, not a selecting a specific one's. |
Pinging @elastic/kibana-app (Team:KibanaApp) |
@alexwizp you are moving me toward the This is a clear improvement over the current state which is an open text input, and I appreciate the need to conduct wildcard searches. I also see @stratoula 's point about not having to memorize index pattern names. My instinct is to continue with I'm not aware of any metrics to support it, but this makes me wonder how often the comobox approach would do the job - perhaps it covers most cases, but we have no way of knowing. Given that, it feels to me that the more open-ended approach does not box us in for now, makes things noticeably better, and affords us the opportunity (and time) to learn from users. |
One additional point - whatever solution we arrive at should likely be the default, shared experience across Kibana in the long run. |
@elasticmachine merge upstream |
const suggestions = availableIndexes.filter( | ||
(index) => | ||
index !== prefix && | ||
index.startsWith(prefix) && |
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.
I suggest to not return suggestions that startWith
the input value but to return suggestions that contain the input value. In that way, I have to remember only one word from my index pattern and not how it starts. For example I write logs
and the kibana_sampl_data_logs
index pattern is suggested. I think that this will improve the UX a lot 🙂
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.
changed that and I like how it works now ❤️
src/plugins/vis_type_timeseries/public/application/components/index_pattern_suggest.tsx
Outdated
Show resolved
Hide resolved
💔 Build Failed
Failed CI StepsTest FailuresChrome X-Pack UI Functional Tests.x-pack/test/functional/apps/rollup_job/tsvb·js.rollup app tsvb integration create rollup tsvbStandard Out
Stack Trace
Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/rollup_job/tsvb·js.rollup app tsvb integration create rollup tsvbStandard Out
Stack Trace
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
@ryankeairns could you please help us to find some workaround or help with fixing the following issue elastic/eui#4345 on Elastic UI side. It's a blocker for that PR, cause for some cases we cannot input the value using the keyboard. |
@myasonik can you assist with this? |
@alexwizp Regarding the accessibility issues a) thank you for keeping this top of mind and pressing for a proper fix and b) we need the EUI team to complete some refactoring in order to address this and other related issues. The good news is that it appears on their 2021 roadmap (see EuiSelectable item), however this won't be done for some time. From what I understand, a workaround is likely to be messy and may be time better spent fixing the underlying issues at the EUI level. Since these same issues exist for the global KQL bar, is it truly blocking? There's no debating it is an undesirable accessibility issue, but perhaps you can proceed with this work. |
Closes: #80849
Summary
Replace indexpattern input with a EuiSuggest component
Related issues in EUI (Not a blockers for that PR):
Screens
Checklist
Delete any items that are not applicable to this PR.