-
Notifications
You must be signed in to change notification settings - Fork 843
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
EuiComboBox #567
EuiComboBox #567
Conversation
07c2327
to
1d34369
Compare
@nreese @snide @peteharverson Could you please take a look at how this behaves in the browser and give me your thoughts before I add tests? @timroes Could you do the same, especially regarding screen reader and keyboard accessibility? Thanks! |
This is great and covers almost all the use cases I can think of (minus the single select one, which I see you have as another item). I'll have some styling to refine it, but all the bits are there so it should be a T-Ball. Big thumbs up. The following are some finesse suggestions.
I know this is a big one so I'm sure I'll have other stuff, but those are the bits that jumped out at me immediately. All that said it's really impressive and mostly there, certainly from a functionality scenario. |
Thanks for creating this. I will try to review (accessibility) as soon as possible. Single select would btw be a "super-nice-to-have", since that would make it usable in most places where we currently use |
@cjcenizal Ideally, I'd port The tag editor on its own is nice because it allows you to focus on that use case, so you get things like the ability to enter random stuff, validation, text editing that feels very natural in the case of insignia (where out the box you can use arrow navigation to edit the tags, for instance), and conversely you can attach the autocompletion component to things like textareas, divs, inputs, or anything you like, making it pretty flexible as well and useful for later use cases like adding user mentions, which is a requirement EUI will definitely get at a certain point in its lifetime, so we might as well start considering those now. |
@bevacqua Thanks for your feedback. It sounds like this component could substitute for some of the use cases of your discrete input component, but it's not quite as flexible as you want. I think it will probably make both development and consumption easier if we pursue the direction you suggest as a separate component. @timroes @jgowdyelastic I'll implement single-selection before merging. Thanks for pointing out its importance. @nreese Per our convo I'll also render the options list within a portal, so EuiComboBox can be used inside of dashboards. @snide Thanks for your comments. To your points:
|
@snide @timroes @jgowdyelastic @nreese I've addressed your comments. Please take another look so I can merge this. I hate to do it but I think tests will have to come separately. |
@cjcenizal I am seeing some weird behavior with the latest version in Kibana. Opening the options list makes the rest of the page content disappear. There are no errors or warnings in the console. Here is my WIP PR that is showing the weird behavior |
@cjcenizal Thanks for the note about the euiBody-hasToolTip rename to euiBody-hasPortalContent. Kibana has a css rule overriding euiBody-hasToolTip position and when that override was applied, the problems above were fixed |
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.
This is great. I ran through the code a bit and it was pretty easy to follow. Really nice work. This one was super tricky.
I set up a PR to utilize EuiBadge
for the pill component and did some other misc. cleanup. Let's us apply coloring so that it'll work better with @nreese's tagging system.
Only bug i noticed was that it breaks a little bit when you utilize EuiFormRow
. The label to input gets busted and the dropdown has trouble disappearing). Heres a gif showing the issue.
}; | ||
} | ||
|
||
onChange = (selectedOption) => { |
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.
This example is a little confusing because selectedOption
is an array. Maybe it would be clearer if its written like
onChange = (selectedOptions) => {
let selectedOption;
if (selectedOptions.length > 0) {
selectedOption = selectedOptions[0];
this.setState({
selectedOption
});
}
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.
Will do.
I think I've run out of time on this. I'm on PTO tomorrow and next week I need to shift my focus to Cloud. Unless anyone objects, I'm going to merge this tomorrow morning. Here are the outstanding issues I know of that still need to be addressed:
Once this is merged I'll break out individual issues for these. |
… show the options list at all.
…u clicked on an option or a pill's close button.
I addressed the bug @nreese found. Merging now! |
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.
Your last commits fixed the problems I was seeing.
lgtm
Replaces #52
Input
Options list
Use cases
Empty states
Keyboard accessibility
hitting left arrow will focus that pillLeft and right arrow keys can be used to navigate through the pillsScreen reader accessibility
aria-label
to read the humanized values aloudAsync actions
isLoading
prop)