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

add disabled prop to EuiComboBoxOption #650

Merged
merged 3 commits into from
Apr 10, 2018

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Apr 10, 2018

I am looking into the feasibility of replacing react-select with EuiComboBox in Kibana Visual Builder. One missing feature is the ability to disable options.

screen shot 2018-04-10 at 1 54 03 pm

This PR just adds a disabled prop to EuiComboBoxOption

@nreese nreese changed the title add disabled prop to EuiComboBox option add disabled prop to EuiComboBoxOption Apr 10, 2018
@nreese nreese force-pushed the disabled_option2 branch from 8bb68bc to c57ed7d Compare April 10, 2018 20:00
@nreese nreese requested review from cjcenizal and snide April 10, 2018 20:01
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.

Looks good! Just had a couple suggestions and then this looks ready to merge to me.

@@ -13,6 +13,7 @@ export default class extends Component {
'data-test-subj': 'titanOption',
}, {
label: 'Enceladus',
disabled: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change the label to "Enceladus is disabled" to make the example a bit clearer?

@@ -16,4 +16,8 @@
color: $euiColorPrimary;
background-color: $euiFocusBackgroundColor;
}
&:disabled {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also add a rule to override the hover state:

  &:disabled {
    color: $euiColorMediumShade;
    cursor: not-allowed;
    &:hover {
      text-decoration: none;
    }
  }

Copy link
Contributor

Choose a reason for hiding this comment

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

Been dealing with these kinds of conflicts a lot lately. Another way to deal with this kind of stuff is to only apply hover states when &:enabled:hover. Cuts out the need for extra specificity overwrites.

No need to do that here, just was a fun realization I had last week and wanted to share. I didn't even know enabled was a thing.

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.

All good after fixing those bits CJ mentioned.

@nreese nreese merged commit e4ec1cd into elastic:master Apr 10, 2018
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.

3 participants