-
Notifications
You must be signed in to change notification settings - Fork 843
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 - remove cross icon when singleSelection is enabled #769
Conversation
nreese
commented
May 3, 2018
CHANGELOG.md
Outdated
@@ -2,6 +2,7 @@ | |||
|
|||
- Added `EuiDescribedFormGroup` component, a wrapper around `EuiFormRow`(s) ([#707](https://github.com/elastic/eui/pull/707)) | |||
- Added `describedByIds` prop to `EuiFormRow` to help with accessibility ([#707](https://github.com/elastic/eui/pull/707)) | |||
- `EuiComboBox` remove cross icon when `singleSelection` is enabled ([#769](https://github.com/elastic/eui/pull/769)) |
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.
Maybe...
Removed individual badge cross icon when
EuiComboBox
hassingleSelection
prop enabled.
When I read this at first I assumed you were removing the all clear cross.
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 also adding the idea of single selection more than just the extra step of removing the badge's cross. May want say something regarding that?
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.
nevermind me....
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.
Can you add a docs section that explains single selection? I'm thinking it will eventually do more than just remove this button.
@@ -47,6 +47,7 @@ export class EuiComboBox extends Component { | |||
options: [], | |||
selectedOptions: [], | |||
isClearable: true, | |||
singleSelection: false, |
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 should be added to the propTypes object too, correct?
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.
it is already a prop type on EuiComboBox. This PR just sets a default value to false and passes the prop to EuiComboBoxInput.
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.
Ah you're right, we may want to think about alphabetizing our props... ha!
@cchaos There is already a docs section on single selection, https://github.com/elastic/eui/blob/master/src-docs/src/views/combo_box/combo_box_example.js#L197. Do you think this needs more details? |
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.
Ugh, nevermind me, it's been a long day....