Add closeOnSelect prop to SelectMenu #588
Conversation
| @@ -38,7 +38,9 @@ export default class OptionsList extends PureComponent { | |||
| /** | |||
| * This holds the values of the options | |||
| */ | |||
| selected: PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.string, PropTypes.number])), | |||
| selected: PropTypes.arrayOf( | |||
There was a problem hiding this comment.
Did you configure your editor? I don't think these lines should be formatted.
There was a problem hiding this comment.
I'm using VS Code. It's picking up the prettier settings in the package.json and as there is no printWidth setting is going with the default of 80. This line is longer than that, which is why it is being formatted.
I've left it as is as the prettier settings would need to explicitly state a larger printWidth. Is this ok? Would you rather I change it back?
There was a problem hiding this comment.
Weird. Maybe nobody had touched the file in a while. Thanks for checking!
| @@ -201,6 +204,9 @@ export default class OptionsList extends PureComponent { | |||
|
|
|||
| handleSelect = item => { | |||
| this.props.onSelect(item) | |||
| if (!this.props.isMultiSelect && this.props.closeOnSelect) { | |||
There was a problem hiding this comment.
I realized that hitting the Enter key has a similar behavior (closing the menu). It makes me wonder if we should also have the check for multiSelect and closeOnSelect in the handleEnter method (you can always close the menu by hitting ESC in that case). Doing this would be a change of contract in the interface. I'm not sure if we want to do that now.
@mshwery - would love some thoughts!
There was a problem hiding this comment.
I think we could save that for a separate PR if the current behavior is problematic. I think it's fine to leave alone for now TBH, though I agree with your idea!
src/select-menu/src/OptionsList.js
Outdated
| @@ -48,7 +50,8 @@ export default class OptionsList extends PureComponent { | |||
| filterPlaceholder: PropTypes.string, | |||
| filterIcon: PropTypes.string, | |||
| optionsFilter: PropTypes.func, | |||
| defaultSearchValue: PropTypes.string | |||
| defaultSearchValue: PropTypes.string, | |||
| closeOnSelect: PropTypes.bool | |||
There was a problem hiding this comment.
While the name is pretty semantic would you mind adding some jsdoc description above the new prop?
There was a problem hiding this comment.
no worries, done
| @@ -201,6 +204,9 @@ export default class OptionsList extends PureComponent { | |||
|
|
|||
| handleSelect = item => { | |||
| this.props.onSelect(item) | |||
| if (!this.props.isMultiSelect && this.props.closeOnSelect) { | |||
There was a problem hiding this comment.
I think we could save that for a separate PR if the current behavior is problematic. I think it's fine to leave alone for now TBH, though I agree with your idea!
This change adds a prop that when true the select menu will close when an option has been selected. closes segmentio#456
robphoenix
force-pushed
the
close-on-select
branch
from
312b0f9
to
68f820a
Compare
|
Thanks @solon-aguiar @mshwery! I've updated my PR & commit message, and added a note to the docs also. |
robphoenix
changed the title
|
No worries, welcome 🎆 |
We need to pull out `closeOnSelect` so that we don't pass it in `props` to the `Pane` in `OptionsList`. Follow on to: #588 | Before | After | | --- | --- | |  |  |
This is a rough WIP to check this is the right approach, or if there is a better way.In response to #456 this change adds a prop to
SelectMenuto enable closing the select menu when an option is selected.