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 with no custom option: bug fix, cleaned up example, added comment #1796

Merged

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Apr 4, 2019

Summary

Resolves #1790

combo box error text

Fixed a bug in determining when the focus had left EuiComboBox and added a form row with error to the Disallowing custom options example explaining to the user why the field is marked invalid.

Checklist

- [ ] This was checked in mobile
- [ ] This was checked in IE11
- [ ] This was checked in dark mode
- [ ] Any props added have proper autodocs
- [ ] Documentation examples were added

  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
    - [ ] Jest tests were updated or added to match the most common scenarios
    - [ ] This was checked against keyboard-only and screenreader scenarios
    - [ ] This required updates to Framer X components

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Tested locally, code LGTM! Thanks for fixing this. Had a couple minor suggestions.

src/components/combo_box/combo_box.js Outdated Show resolved Hide resolved
src/components/combo_box/combo_box.js Outdated Show resolved Hide resolved
@cchaos
Copy link
Contributor

cchaos commented Apr 4, 2019

My only concern is that when you tab back into it (Shift + tab), while the input stays in an invalid state the help text is gone.

@chandlerprall
Copy link
Contributor Author

@cchaos updated to clear the error state only when the options change, or the field is re-blurred + the input is empty

@cchaos
Copy link
Contributor

cchaos commented Apr 11, 2019

Hey, sorry one more case where if you close the dropdown via tabbing and selecting the arrow, the error message doesn't change correctly.

@chandlerprall chandlerprall requested a review from cjcenizal April 18, 2019 16:07
@chandlerprall
Copy link
Contributor Author

@cchaos @cjcenizal updated to have two validation paths: first is the existing onBlur, second is when the value changes, it checks to see if there are any matching options.

I found a separate issue, when the input has an exact match but additional partial match(es), the wording is incorrect:

validate options

titan is valid but still not selected. I think the onBlur text should be updated to something like "titan" was not saved but I can't come up with exact wording that I like.

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.

I'm good with how it's working now. The only possibly odd thing is using the dropdown arrow will show the error message but it won't change the state of the input until you click outside of it.

But, I can probably live with that? The only worry would be if the consumer doesn't provide an error message. So maybe I'm back-tracking and that should be fixed as well?

but I can't come up with exact wording that I like

I'd just leave the error message as it is. It's only the docs anyway

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

LGTM! Great work.

@chandlerprall
Copy link
Contributor Author

@cchaos updated to also indicate invalid state when the dropdown arrow is clicked

@chandlerprall chandlerprall merged commit d6a299e into elastic:master Apr 26, 2019
@chandlerprall chandlerprall deleted the bug/1790-combobox-with-entry branch April 26, 2019 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EuiComboBox shouldn't show as invalid unless told to by owner
3 participants