EuiComboBox additions #1139
EuiComboBox additions #1139
Conversation
with `asPlainText` as a property of `singleSelection`
| @@ -48,7 +48,7 @@ export default class extends Component { | |||
| return ( | |||
| <EuiComboBox | |||
| placeholder="Select a single option" | |||
| singleSelection={true} | |||
| singleSelection={{ asPlainText: true }} | |||
There was a problem hiding this comment.
It seems like this will usually be the desired case, right? I can't think of a situation where we'd want the pill form for single selection. If you agree then how about we update the combo box to use asPlainText: true as default, while still giving people the option to specify false if for some reason they want the pill?
| ...rest | ||
| } = option; | ||
|
|
||
| const asPlainText = singleSelection && singleSelection.asPlainText; |
There was a problem hiding this comment.
Here's how we can set asPlainText to default to true:
// Default asPlainText to true if using singleSelection.
const asPlainText = singleSelection && singleSelection.asPlainText !== false;Note that this will be a breaking change for anyone who is depending upon the pill form so we'll need to update the CHANGELOG.md.
| @@ -31,7 +31,10 @@ export class EuiComboBox extends Component { | |||
| placeholder: PropTypes.string, | |||
| isLoading: PropTypes.bool, | |||
| async: PropTypes.bool, | |||
| singleSelection: PropTypes.bool, | |||
| singleSelection: PropTypes.oneOfType([ | |||
There was a problem hiding this comment.
Can we change this propType to also check for the asPlainText property, both here and in combo_box_input.js?
singleSelection: PropTypes.oneOfType([
PropTypes.bool,
PropTypes.shape({
asPlainText: PropTypes.bool,
}),
]),
|
Thanks for taking a look @cjcenizal. I thought about making it true by default as well, but I really didn't want to have to introduce a breaking change for such a small change. It also still acts a little oddly when just showing in plain text, as the user can keep typing to filter down the list and select a different option. This is odd behavior because by making the selection render plain text, it looks like a normal select, but doesn't act like one. So it's not 100%, and so I wouldn't want to break consumer implementation. |
I didn't notice that, good point. |
| singleSelection: PropTypes.bool, | ||
| singleSelection: PropTypes.oneOfType([ | ||
| PropTypes.bool, | ||
| PropTypes.object, |
There was a problem hiding this comment.
Can we use PropTypes.shape here too?
| @@ -57,13 +61,12 @@ export class EuiComboBoxPill extends Component { | |||
| ); | |||
| } | |||
|
|
|||
| if (asPlainText) { | |||
| return <span className={classes}>{children}</span>; | |||
There was a problem hiding this comment.
should this also pass ...rest ?
There was a problem hiding this comment.
Youp, I'll add it. Good catch.
|
@chandlerprall Can you check if I updated the TS defs correctly? https://github.com/elastic/eui/pull/1139/files#diff-4354bb6c320e9f824ccf47162afe5dd8 |
|
@cchaos TS change looks good, thanks for making it! |
|
Sweet! Thx! |
When working on #1137, I needed some extra functionality from the
EuiComboBox, these are those edits.1. Allowing
onClickto be passed to whole pill2. Allowing single selection types to show plain text via
asPlainTextas a property ofsingleSelectionIt also makes sure that single selection types don't grow in height.
3. Altering the padding based on props. It used to always be large (2 icon) padding, even if the box wasn't clearable (therefore only had 1 icon).
Before
After