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 converts entered text into a custom option on blur #1353

Merged
merged 5 commits into from
Dec 7, 2018

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Dec 6, 2018

Fixes #1307

When I test this out locally the UX feels pretty intuitive to me. What does everyone else think?

combo_box_input_on_blur

… user takes away focus, e.g. by tabbing to another element. This prevents the `EuiComboBox` from being mistaken for a `EuiInputText`.
@snide
Copy link
Contributor

snide commented Dec 7, 2018

@cjcenizal Assuming this is the way we want to go with this, should it also perform this on a tab out?

@cjcenizal
Copy link
Contributor Author

Assuming this is the way we want to go with this, should it also perform this on a tab out?

Yup, I think so. It's hard to tell but the first blur event in the gif is caused by a tab. The change is triggered on any blur event, regardless of whether it's triggered by tab, clicking outside of the element, or done programmatically.

@snide
Copy link
Contributor

snide commented Dec 7, 2018

@cjcenizal Oh, cuz i see it not working with tab, which is why i asked. Meaning, that's how it seems to work when I test it.

@snide
Copy link
Contributor

snide commented Dec 7, 2018

Disregard. I think it was just validation of dupes that caused it. Hmm. I wonder if we should clear the input on blur then if for whatever reason it wouldn't complete (normally a validation bit)

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

I'm a little cautious about this creating accidental items, but in the end this is similar to what would happen with a text field that you half filled out. The only place i think it could be problematic is if you have data writing immediately on blur rather than through a form submit. I could see someone possibly wanting to disable this, but I'm OK waiting to see if that's a real problem or jus my own paranoia.

@cjcenizal cjcenizal requested a review from cchaos December 7, 2018 04:42
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Works for me

CHANGELOG.md Outdated
@@ -3,6 +3,7 @@
- Altered functionality of `truncate` on `EuiBreadcrumbs` and added `truncate` ability on breadcrumb item ([#1346](https://github.com/elastic/eui/pull/1346))
- Altered `EuiHeader`'s location of `EuiHeaderBreadcrumbs` based on the new `truncate` ability ([#1346](https://github.com/elastic/eui/pull/1346))
- Added support for `href` and `target` props in `EuiBasicTable` actions ([#1347](https://github.com/elastic/eui/pull/1347))
- `EuiComboBox` will convert entered text into a custom option when the user takes away focus, e.g. by tabbing to another element. This prevents the `EuiComboBox` from being mistaken for a `EuiInputText`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can turn this into an "Added support for..." entry.

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.

LGTM

CHANGELOG.md Outdated
@@ -12,6 +13,7 @@
**Bug fixes**

- Fixed EUI when used in an environment lacking ES Modules support, e.g. Jest ([#1358](https://github.com/elastic/eui/pull/1358))
>>>>>>> upstream/master
Copy link
Contributor

Choose a reason for hiding this comment

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

bad merge resolution

@cjcenizal cjcenizal merged commit f7e447e into elastic:master Dec 7, 2018
@cjcenizal cjcenizal deleted the combo-box-add-input-on-tab branch December 7, 2018 20:46
@snide snide mentioned this pull request Dec 7, 2018
3 tasks
snide added a commit to snide/eui that referenced this pull request Dec 11, 2018
snide added a commit that referenced this pull request Dec 11, 2018
cjcenizal added a commit to cjcenizal/eui that referenced this pull request Dec 11, 2018
cjcenizal added a commit that referenced this pull request Dec 11, 2018
…al implementation (#1364)

* Don't call onCreateOption callback when input is empty.
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.

4 participants