-
Notifications
You must be signed in to change notification settings - Fork 841
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
Adds isDisabled prop to EuiComboBox #829
Conversation
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.
LGTM
This looks great, thanks for the quick turnaround! I left a small comment; feel free to address or ignore it.
@@ -578,12 +582,13 @@ export class EuiComboBox extends Component { | |||
autoSizeInputRef={this.autoSizeInputRef} | |||
inputRef={this.searchInputRef} | |||
updatePosition={this.updateListPosition} | |||
onClear={isClearable && this.clearSelectedOptions ? this.clearSelectedOptions : undefined} | |||
onClear={(isClearable && this.clearSelectedOptions && !isDisabled) ? this.clearSelectedOptions : undefined} |
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.
What are your thoughts on pulling this out into a function? I feel like it might improve the readability now that there are three conditions being checked here.
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.
lgtm
jenkins test this |
Fixes #822
Adds
isDisabled
prop toEuiComboBox
. Will properly disable the input and remove button actions on the pills, clears and dropdown arrows.This does remove the item from screen readers since it isn't focusable. I tried a couple options but this is something sort of built into HTML and acts similar to our other inputs. I'm going to make an issue to try and think up something clever to fix this across our forms in general.