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.

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 now gives focus to the search input when the user clicks the clear button #918

Merged
merged 1 commit into from
Jun 12, 2018

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Jun 11, 2018

This prevents focus from defaulting to the body. It's an accessibility problem if focus suddenly goes to the body element after clicking a button. The behavior introduced by this PR is also how react-select solves this problem.

I also decorated EuiComboBox with data-test-subj selectors for the search input and clear button and tried to make the tests and snapshots a bit clearer regarding what's being tested.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Code change LGTM; nice test cases too!

@nreese
Copy link
Contributor

nreese commented Jun 12, 2018

I have a question about adding the data-test-subj directly to EUI. Can users override these? How should testing be handled when there are more than one combo box on a page?

@cjcenizal
Copy link
Contributor Author

@nreese The user can't override those data-test-subj attributes, but you can provide a unique data-test-subj to EuiComboBox itself, and then create a selector that uses that. This will let you manipulate multiple combo boxes on the page.

<EuiComboBox data-test-subj="myComboBox" />

document.querySelector('[data-test-subj="myComboBox"] [data-test-subj="comboBoxToggleListButton"]');

…s the clear button, to prevent focus from defaulting to the body.

* EuiComboBox is now decorated with data-test-subj selectors for the search input, toggle list button, and clear button.
* Updated snapshots.
@cjcenizal cjcenizal force-pushed the combo-box-clear-gives-focus branch from 756a735 to 2348536 Compare June 12, 2018 15:31
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.

lgtm

code review
tested combo box in EUI help pages

@cjcenizal cjcenizal merged commit 6b0fce3 into elastic:master Jun 12, 2018
@cjcenizal cjcenizal deleted the combo-box-clear-gives-focus branch June 12, 2018 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants