[Labs] New MultiSelect component #1275
Conversation
Merge remote-tracking branch 'origin/master' into al/multiselect2Preview: documentation |
Fix TagInput styles in dark themePreview: documentation |
| onKeyUp={this.state.isOpen && handleKeyUp} | ||
| > | ||
| <TagInput | ||
| inputProps={defaultInputProps} |
There was a problem hiding this comment.
oof this'll get clobbered if the user defines even one custom inputProp... gonna have to spread tagInputProps.inputProps into defaultProps.
|
|
||
| public componentDidMount() { | ||
| // can't use defaultProps because "static members | ||
| // cannot reference class type parameters" |
There was a problem hiding this comment.
i'm sure you can use defaultProps, but you probably don't need to. safe to remove this comment as it doesn't really make sense on its own (def makes sense in PR description tho).
think two months from now--will we even know what this means? why it matters?
There was a problem hiding this comment.
That's the problem I'm hitting:
I'd still like to be able to set default props for openOnKeyDown and resetOnSelect
| public componentDidMount() { | ||
| // can't use defaultProps because "static members | ||
| // cannot reference class type parameters" | ||
| this.setState({ selectedItems: this.props.selectedItems || [] }); |
There was a problem hiding this comment.
going to need to update state based on prop in componentWillReceiveProps too (you've only done the initial render, not all subsequent renders).
but actually, i would kill the state field entirely and make it only controlled via props.
| activeItem?: T; | ||
| isOpen?: boolean; | ||
| query?: string; | ||
| selectedItems?: T[]; |
There was a problem hiding this comment.
i would argue for removing this from state entirely and supporting only controlled usage. the other state fields are about internal interactions, but selectedItems is the big deal and the user will want to track it anyway. this'll make usage and testing much simpler and more reliable.
note how Select<T> doesn't even have a prop for its value, just onItemSelect event handler for user to receive updates and manage their own state.
There was a problem hiding this comment.
perhaps echo TagInput's API: onItemAdd, onItemRemove.
There was a problem hiding this comment.
It's fully controlled now, let me know what you think
| Utils.safeInvoke(this.props.tagInputProps.onRemove, _item, index); | ||
| } | ||
|
|
||
| private handleItemSelect = (item: T, _event: React.SyntheticEvent<HTMLElement>) => { |
There was a problem hiding this comment.
no need for _ prefix cuz it's used on the last line. same with handleTagRemove above.
| // the input is no longer focused so we can close the popover | ||
| let nextState = { isOpen: false }; | ||
|
|
||
| if (this.props.resetOnSelect) { |
There was a problem hiding this comment.
hmmm this logic happens at least three times. i'm sure we can come up with a helpful util to handle it only once, but i don't have the mental space for that right now 🌴
There was a problem hiding this comment.
Let me know if you think of anything 🥇
|
|
||
| private handleKeyDown = (event: React.KeyboardEvent<HTMLElement>) => { | ||
| if (event.which === Keys.ESCAPE | ||
| || event.which === Keys.TAB) { |
There was a problem hiding this comment.
one line, def not long enough for two. avoid awkward alignment.
| this.setState(nextState); | ||
| } else if (!(event.which === Keys.ARROW_LEFT | ||
| || event.which === Keys.ARROW_RIGHT | ||
| || event.which === Keys.BACKSPACE)) { |
There was a problem hiding this comment.
deconstructing which might let this all fit on one line...
which === Keys.ARROW_LEFT || which === Keys.ARROW_RIGHT || which === Keys.BACKSPACE
There was a problem hiding this comment.
but actually TagInput calls preventDefault() when those keys are triggered... maybe you can just check event.isDefaultPrevented instead of redefining this logic?
There was a problem hiding this comment.
Good catch, seems to be working!
There was a problem hiding this comment.
Nevermind, event.isDefaultPrevented didn't work out so I had to put back those 3 keys
| } | ||
|
|
||
| if (this.queryListKeyDown) { | ||
| Utils.safeInvoke(this.queryListKeyDown, event); |
There was a problem hiding this comment.
oh snap i see what's going on here. listProps.handleKeyDown is only accessible inside the callback. ok, c'est la vie 🌴
| * Whether the filtering state should be reset to initial when an item is selected | ||
| * (immediately before `onItemSelect` is invoked), or when the popover closes. | ||
| * The query will become the empty string and the first item will be made active. | ||
| * @default true |
There was a problem hiding this comment.
i think this should default to false. it's quite jarring when it's on.
There was a problem hiding this comment.
Nah, check it out again. Makes more sense to reset the query after selection, because you're then ready to search again without having to manually clear the field. Select2 and react-select also have this default
|
@llorca there is some text overlapping in the droppdown:
|
Remove selected items internal state, fully controlled approachPreview: documentation |
Better logic for opening/closing popoverPreview: documentation | table |
|
A couple interaction questions:
|
| @@ -0,0 +1,228 @@ | |||
| /* | |||
There was a problem hiding this comment.
i think both these files belong in the select/ directory as they share interfaces and concepts
Move files to `select/` directoryPreview: documentation | table |
|
Properly reset activeItem after selection, if necessaryPreview: documentation | table |
Update import pathsPreview: documentation |
packages/labs/src/common/classes.ts
Outdated
| @@ -5,6 +5,7 @@ | |||
| * and https://github.com/palantir/blueprint/blob/master/PATENTS | |||
| */ | |||
|
|
|||
| export const MULTISELECT = "pt-multiselect"; | |||
There was a problem hiding this comment.
this should be pt-multi-select
| popoverProps, | ||
| resetOnSelect, | ||
| tagInputProps, | ||
| ...props, |
There was a problem hiding this comment.
nit: would prefer naming this restProps for clarity through verbosity
| } | ||
|
|
||
| private renderQueryList = (listProps: IQueryListRendererProps<T>) => { | ||
| const { tagInputProps = {}, popoverProps = {} } = this.props; |
There was a problem hiding this comment.
minor: set these to {} with public static defaultProps instead of as destructuring defaults
There was a problem hiding this comment.
Yeah that's what I wanted, but hit a road block. We went over this earlier in this thread. That's the problem I'm hitting:
I also want to be able to set default props for openOnKeyDown and resetOnSelect
There was a problem hiding this comment.
ah, I can see that Select has this code issue as well. we can address them both outside this PR.
| const { handleKeyDown, handleKeyUp, query } = listProps; | ||
| const defaultInputProps: HTMLInputProps = { | ||
| placeholder: "Search...", | ||
| ...this.props.tagInputProps.inputProps, |
There was a problem hiding this comment.
this looks like it should be tagInputProps.inputProps, otherwise, this could throw a null pointer exception
| const defaultInputProps: HTMLInputProps = { | ||
| placeholder: "Search...", | ||
| ...this.props.tagInputProps.inputProps, | ||
| // tslint:disable-next-line:object-literal-sort-keys |
There was a problem hiding this comment.
looks like tslint is outdated (I believe we fixed this bug). but this is fine for this PR
| value: query, | ||
| }; | ||
|
|
||
| this.queryListKeyDown = handleKeyDown; |
There was a problem hiding this comment.
a little strange to see a class member being assigned in a render method... what's going on here?
There was a problem hiding this comment.
That handleKeyDown is given to my renderQueryList function by the renderer prop of QueryList, therefore I don't have access to it outside of this function even though I need it. Had to store it in a class member, any better option?
There was a problem hiding this comment.
meh yeah this is kind of hairy. would prefer to make an exception to the jsx lambda rule here and refactor this.handleKeyDown into a getter:
<div
onKeyDown={this.getTargetKeyDownHandler(listProps.handleKeyDown)}
onKeyUp={this.state.isOpen && handleKeyUp}
>
...
private getTargetKeyDownHandler(handleQueryListKeyDown: React.EventHandler<React.KeyboardEvent<HTMLElement>>) {
return (e: React.KeyboardEvent<HTMLElement>) => {
const { which } = e;
const { resetOnSelect } = this.props;
...
if (this.state.isOpen) {
Utils.safeInvoke(handleQueryListKeyDown, e);
}
};
}| private handleItemSelect = (item: T, event: React.SyntheticEvent<HTMLElement>) => { | ||
| this.input.focus(); | ||
| // make sure the query is valid by checking if activeItem is defined | ||
| if (this.state.activeItem) { |
There was a problem hiding this comment.
please compare against null (this.state.activeItem != null), don't implicitly coerce to boolean
| this.setState({ query, isOpen: !(query.length === 0 && this.props.openOnKeyDown) }); | ||
| } | ||
|
|
||
| private handleItemSelect = (item: T, event: React.SyntheticEvent<HTMLElement>) => { |
There was a problem hiding this comment.
don't name events event (annoyingly, it conflicts with window.event), just go with e here
|
|
||
| private handlePopoverInteraction = (nextOpenState: boolean) => requestAnimationFrame(() => { | ||
| // deferring to rAF to get properly updated activeElement | ||
| const { popoverProps = {}, resetOnSelect } = this.props; |
There was a problem hiding this comment.
again use defaultProps so that you're not setting the default in multiple places
| query: resetOnSelect ? "" : this.state.query, | ||
| }); | ||
| } else if (!(which === Keys.BACKSPACE || which === Keys.ARROW_LEFT || which === Keys.ARROW_RIGHT)) { | ||
| this.setState({ isOpen: true }); |
There was a problem hiding this comment.
expected 4-space indentation
There was a problem hiding this comment.
How does this even happen
Address nitsPreview: documentation | table |
Refactor class member into getterPreview: documentation |
Fix docs URLPreview: documentation |
Fixes #118
Checklist
Changes proposed in this pull request:
Final piece of #118. The new
MultiSelect<T>component is similar toSelect<T>. It composesQueryList<T>andTagInputtogether. Usage is very similar toSelect<T>.Reviewers should focus on:
Static members cannot reference class type parameters, so for now I had to resort tocomponentDidMount.selectedItems. I'm far behind on algorithms and data structures, what can be done better here?QueryList<T>. There's too much duplicated code acrossSelect<T>andMultiSelect<T>at the moment.Screenshot