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

Update combo box components to React 16.3 lifecycle #890

Merged

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented May 30, 2018

Part of #763

EuiComboBoxOptionsList and EuiComboBoxInput were straight-foward changes. I moved two class members on EuiComboBox into its state. Updating matchingOptions 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.

@chandlerprall chandlerprall requested a review from nreese May 30, 2018 22:31
@nreese
Copy link
Contributor

nreese commented May 30, 2018

The clear button in the Single selection example does not work.

@chandlerprall
Copy link
Contributor Author

@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

  1. its disabled (existing functionality)
  2. there is no onClear prop
  3. there are no selected items

@chandlerprall chandlerprall requested a review from snide May 31, 2018 16:49
@nreese
Copy link
Contributor

nreese commented May 31, 2018

@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.

@snide
Copy link
Contributor

snide commented May 31, 2018

Clear logic should be

isClearable=true, disabled={false}, and items.length > 0

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.

@chandlerprall
Copy link
Contributor Author

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 clear prop is present, even if clear.onClick is undefined.

The issue @nreese discovered with single selection is because it sets isClearable={false} which comes into this function as an undefined onClear, and similarly having no selected options prevents onClick from getting a value.

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) {
Copy link
Contributor

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

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

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.

Run locally and tested for functionality. Looks good.

@chandlerprall chandlerprall merged commit a82ea7a into elastic:master Jun 1, 2018
@chandlerprall chandlerprall deleted the react-16-3-c-c-c-combobox branch June 1, 2018 16:28
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.

None yet

3 participants