-
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
Update combo box components to React 16.3 lifecycle #890
Update combo box components to React 16.3 lifecycle #890
Conversation
The clear button in the |
@nreese good catch. Looks like combo box had an existing bug where the clear button was always displayed unless it was disabled. I've pushed up a fix to not show the clear button if
|
@chandlerprall I do not think "Looks like combo box had an existing bug where the clear button was always displayed unless it was disabled" is a bug. That is the intended behavior. The clear button should be displayed by default - unless it is disabled. |
Clear logic should be
We should not UI that doesn't provide an action. Better to have it appear when it has a use. Don't think we need the affordance in this case. |
Just saw I'd failed to push the additional change so it was quite difficult for anyone to see. It's at combo_box_input.js:152. The clear icon is displayed when The issue @nreese discovered with single selection is because it sets The change shifts the UI button's display logic to match what @snide said in the above comment. |
@@ -150,9 +149,9 @@ export class EuiComboBoxInput extends Component { | |||
|
|||
const clickProps = {}; | |||
|
|||
if (!isDisabled) { | |||
if (!isDisabled && onClear && hasSelectedOptions) { |
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 a lot cleaner - nice
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
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.
Run locally and tested for functionality. Looks good.
Part of #763
EuiComboBoxOptionsList
andEuiComboBoxInput
were straight-foward changes. I moved two class members onEuiComboBox
into its state. UpdatingmatchingOptions
state value in componentDidUpdate could create an infinite loop so I added a quick check to see if the matching options have changed. Existing tests pass and the examples work identically to the published docs.