-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[MultiSelect] Enable pasting of multiple values (same as TagInput) #3428
Changes from 7 commits
28c8207
09874bb
0fe9cb5
b92350d
57a40ec
9fb0d71
079c903
3439afd
9fe7912
e41e4bc
a739750
e89272a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,17 @@ import * as Utils from "../../common/utils"; | |
import { Icon, IconName } from "../icon/icon"; | ||
import { ITagProps, Tag } from "../tag/tag"; | ||
|
||
/** | ||
* A type reflecting the manner in which a `TagInput` value was added. | ||
* - `"default"` - indicates that a value was added in the default fasion, via | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: "fashion" actually I would just simplify, remove the metaphor, and say "... was added by manual selection" |
||
* manual selection. | ||
* - `"blur"` - indicates that a value was added when the `TagInput` lost focus. | ||
* This is only possible when `addOnBlur=true`. | ||
* - `"paste"` - indicates that a value was added via paste. This is only | ||
* possible when `addOnPaste=true`. | ||
*/ | ||
export type TagInputAddMethod = "default" | "blur" | "paste"; | ||
|
||
export interface ITagInputProps extends IIntentProps, IProps { | ||
/** | ||
* If true, `onAdd` will be invoked when the input loses focus. | ||
|
@@ -74,7 +85,7 @@ export interface ITagInputProps extends IIntentProps, IProps { | |
* returns `false`. This is useful if the provided `value` is somehow invalid and should | ||
* not be added as a tag. | ||
*/ | ||
onAdd?: (values: string[]) => boolean | void; | ||
onAdd?: (values: string[], method: TagInputAddMethod) => boolean | void; | ||
|
||
/** | ||
* Callback invoked when new tags are added or removed. Receives the updated list of `values`: | ||
|
@@ -260,10 +271,10 @@ export class TagInput extends AbstractPureComponent<ITagInputProps, ITagInputSta | |
); | ||
} | ||
|
||
private addTags = (value: string) => { | ||
private addTags = (value: string, method: TagInputAddMethod = "default") => { | ||
const { inputValue, onAdd, onChange, values } = this.props; | ||
const newValues = this.getValues(value); | ||
let shouldClearInput = Utils.safeInvoke(onAdd, newValues) !== false && inputValue === undefined; | ||
let shouldClearInput = Utils.safeInvoke(onAdd, newValues, method) !== false && inputValue === undefined; | ||
// avoid a potentially expensive computation if this prop is omitted | ||
if (Utils.isFunction(onChange)) { | ||
shouldClearInput = onChange([...values, ...newValues]) !== false && shouldClearInput; | ||
|
@@ -342,7 +353,7 @@ export class TagInput extends AbstractPureComponent<ITagInputProps, ITagInputSta | |
// defer this check using rAF so activeElement will have updated. | ||
if (!currentTarget.contains(document.activeElement)) { | ||
if (this.props.addOnBlur && this.state.inputValue !== undefined && this.state.inputValue.length > 0) { | ||
this.addTags(this.state.inputValue); | ||
this.addTags(this.state.inputValue, "blur"); | ||
} | ||
this.setState({ activeIndex: NONE, isInputFocused: false }); | ||
} | ||
|
@@ -367,7 +378,7 @@ export class TagInput extends AbstractPureComponent<ITagInputProps, ITagInputSta | |
let activeIndexToEmit = activeIndex; | ||
|
||
if (event.which === Keys.ENTER && value.length > 0) { | ||
this.addTags(value); | ||
this.addTags(value, "default"); | ||
} else if (selectionEnd === 0 && this.props.values.length > 0) { | ||
// cursor at beginning of input allows interaction with tags. | ||
// use selectionEnd to verify cursor position and no text selection. | ||
|
@@ -405,7 +416,7 @@ export class TagInput extends AbstractPureComponent<ITagInputProps, ITagInputSta | |
} | ||
|
||
event.preventDefault(); | ||
this.addTags(value); | ||
this.addTags(value, "paste"); | ||
}; | ||
|
||
private handleRemoveTag = (event: React.MouseEvent<HTMLSpanElement>) => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,10 +14,20 @@ | |
export type ItemListPredicate<T> = (query: string, items: T[]) => T[]; | ||
|
||
/** | ||
* Customize querying of individual items. Return `true` to keep the item, `false` to hide. | ||
* This method will be invoked once for each item, so it should be performant. For more complex | ||
* queries, use `itemListPredicate` to operate once on the entire array. | ||
* Customize querying of individual items. | ||
* | ||
* If defined with `itemListPredicate`, this prop will be ignored. | ||
* __Filtering a list of items.__ This function is invoked to filter the list of | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's weird to have the exact same documentation block in two places. it's especially weird to reference the prop names here, when we're not in a react component class module... can you simplify this to a simple one-sentence description and tell people to "see its usage sites in |
||
* items as a query is typed. Return `true` to keep the item, or `false` to | ||
* hide. This method is invoked once for each item, so it should be performant. | ||
* For more complex queries, use `itemListPredicate` to operate once on the | ||
* entire array. For the purposes of filtering the list, this prop is ignored if | ||
* `itemListPredicate` is also defined. | ||
* | ||
* __Matching a pasted value to an item.__ This function is also invoked to | ||
* match a pasted value to an existing item if possible. In this case, the | ||
* function will receive `exactMatch=true`, and the function should return true | ||
* only if the item _exactly_ matches the query. For the purposes of matching | ||
* pasted values, this prop will be invoked even if `itemListPredicate` is | ||
* defined. | ||
*/ | ||
export type ItemPredicate<T> = (query: string, item: T, index?: number) => boolean; | ||
export type ItemPredicate<T> = (query: string, item: T, index?: number, exactMatch?: boolean) => boolean; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,6 +54,24 @@ export interface IQueryListRendererProps<T> // Omit `createNewItem`, because it | |
*/ | ||
handleItemSelect: (item: T, event?: React.SyntheticEvent<HTMLElement>) => void; | ||
|
||
/** | ||
* Handler that should be invoked when the user pastes one or more values. | ||
* | ||
* This callback will use `itemPredicate` with `exactMatch=true` to find a | ||
* subset of `items` exactly matching the pasted `values` provided, then it | ||
* will invoke `onItemsPaste` with those found items. Each pasted value that | ||
* does not exactly match an item will be ignored. | ||
* | ||
* If creating items is enabled (by providing both `createNewItemFromQuery` | ||
* and `createNewItemRenderer`), then pasted values that do not exactly | ||
* match an existing item will emit a new item as created via | ||
* `createNewItemFromQuery`. | ||
* | ||
* If `itemPredicate` returns multiple matching items for a particular | ||
* `value`, then only the first matching item will be emitted. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you mean "for a particular query"? if not, what is |
||
*/ | ||
handlePaste: (values: string[]) => void; | ||
|
||
/** | ||
* Keyboard handler for up/down arrow keys to shift the active item. | ||
* Attach this handler to any element that should support this interaction. | ||
|
@@ -152,6 +170,7 @@ export class QueryList<T> extends React.Component<IQueryListProps<T>, IQueryList | |
handleItemSelect: this.handleItemSelect, | ||
handleKeyDown: this.handleKeyDown, | ||
handleKeyUp: this.handleKeyUp, | ||
handlePaste: this.handlePaste, | ||
handleQueryChange: this.handleQueryChange, | ||
itemList: itemListRenderer({ | ||
...spreadableState, | ||
|
@@ -350,6 +369,40 @@ export class QueryList<T> extends React.Component<IQueryListProps<T>, IQueryList | |
} | ||
}; | ||
|
||
private handlePaste = (values: string[]) => { | ||
const { createNewItemFromQuery, onItemsPaste } = this.props; | ||
|
||
let nextActiveItem: T | undefined; | ||
|
||
// Find an exising item that exactly matches each pasted value, or | ||
// create a new item if possible. Ignore unmatched values if creating | ||
// items is disabled. | ||
const pastedItemsToEmit = values.reduce<T[]>((agg, value) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I would slightly prefer this as a regular for loop, since that would be easier to read than a |
||
const equalItem = getMatchingItem(value, this.props); | ||
|
||
if (equalItem !== undefined) { | ||
nextActiveItem = equalItem; | ||
agg.push(equalItem); | ||
} else if (this.canCreateItems()) { | ||
const newItem = Utils.safeInvoke(createNewItemFromQuery, value); | ||
if (newItem !== undefined) { | ||
agg.push(newItem); | ||
} | ||
} | ||
|
||
return agg; | ||
}, []); | ||
|
||
// UX nicety: update the active item if we matched with at least one | ||
// existing item. | ||
if (nextActiveItem !== undefined) { | ||
this.setActiveItem(nextActiveItem); | ||
} | ||
|
||
// No need to update the active item. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's this comment about? you may have updated the active item at this point, no? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Old cruft. Removed. 👍 |
||
Utils.safeInvoke(onItemsPaste, pastedItemsToEmit); | ||
}; | ||
|
||
private handleKeyDown = (event: React.KeyboardEvent<HTMLElement>) => { | ||
const { keyCode } = event; | ||
if (keyCode === Keys.ARROW_UP || keyCode === Keys.ARROW_DOWN) { | ||
|
@@ -418,10 +471,8 @@ export class QueryList<T> extends React.Component<IQueryListProps<T>, IQueryList | |
} | ||
|
||
private isCreateItemRendered(): boolean { | ||
const { createNewItemFromQuery } = this.props; | ||
return ( | ||
createNewItemFromQuery != null && | ||
this.props.createNewItemRenderer != null && | ||
this.canCreateItems() && | ||
this.state.query !== "" && | ||
// this check is unfortunately O(N) on the number of items, but | ||
// alas, hiding the "Create Item" option when it exactly matches an | ||
|
@@ -430,6 +481,10 @@ export class QueryList<T> extends React.Component<IQueryListProps<T>, IQueryList | |
); | ||
} | ||
|
||
private canCreateItems(): boolean { | ||
return this.props.createNewItemFromQuery != null && this.props.createNewItemRenderer != null; | ||
} | ||
|
||
private wouldCreatedItemMatchSomeExistingItem() { | ||
// search only the filtered items, not the full items list, because we | ||
// only need to check items that match the current query. | ||
|
@@ -443,6 +498,20 @@ function pxToNumber(value: string | null) { | |
return value == null ? 0 : parseInt(value.slice(0, -2), 10); | ||
} | ||
|
||
function getMatchingItem<T>(query: string, { items, itemPredicate }: IQueryListProps<T>): T | undefined { | ||
if (Utils.isFunction(itemPredicate)) { | ||
// .find() doesn't exist in ES5. Alternative: use a for loop instead of | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIK, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @adidahiya Yep, the compile step was failing on Circle. Here is the failing build from a few commits back. lerna ERR! src/components/query-list/queryList.tsx:384:67 - error TS2339: Property 'find' does not exist on type 'T[]'.
lerna ERR!
lerna ERR! 384 itemEqualsQuery === undefined ? undefined : items.find(item => itemEqualsQuery(item, value)); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's unfortunate, I wish we had added it to the environment requirements. oh well. |
||
// .filter() so that we can return as soon as we find the first match. | ||
for (let i = 0; i < items.length; i++) { | ||
const item = items[i]; | ||
if (itemPredicate(query, item, i, true)) { | ||
return item; | ||
} | ||
} | ||
} | ||
return undefined; | ||
} | ||
|
||
function getFilteredItems<T>(query: string, { items, itemPredicate, itemListPredicate }: IQueryListProps<T>) { | ||
if (Utils.isFunction(itemListPredicate)) { | ||
// note that implementations can reorder the items here | ||
|
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.
nit: instead of "reflecting the manner ...", simpler language would be "The method in which a TagInput value was added."