-
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 5 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 |
---|---|---|
|
@@ -34,6 +34,13 @@ export interface IListItemsProps<T> extends IProps { | |
/** Array of items in the list. */ | ||
items: T[]; | ||
|
||
/** | ||
* Whether the provided item fully matches the provided value. This is used | ||
* to match pasted values to existing items, so that items can be emitted | ||
* via `onItemsPaste`. | ||
*/ | ||
itemEqualsQuery?: (item: T, query: string) => boolean; | ||
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. isn't this just i don't see the need for a new prop here, except that it slightly changes the semantics 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. @giladgray They're unfortunately different:
Possible to add an EDIT: Some examples: Example 1. If I paste "The Godfather," then Example 2. If I paste "a" with the "Create item" option enabled, then |
||
|
||
/** | ||
* Specifies how to test if two items are equal. By default, simple strict | ||
* equality (`===`) is used to compare two items. | ||
|
@@ -130,6 +137,11 @@ export interface IListItemsProps<T> extends IProps { | |
*/ | ||
onItemSelect: (item: T, event?: React.SyntheticEvent<HTMLElement>) => void; | ||
|
||
/** | ||
* Callback invoked when multiple items are selected at once via pasting. | ||
*/ | ||
onItemsPaste?: (items: T[]) => void; | ||
|
||
/** | ||
* Callback invoked when the query string changes. | ||
*/ | ||
|
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 `itemEqualsQuery` 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 `itemEqualsQuery` 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,19 @@ function pxToNumber(value: string | null) { | |
return value == null ? 0 : parseInt(value.slice(0, -2), 10); | ||
} | ||
|
||
function getMatchingItem<T>(query: string, { items, itemEqualsQuery }: IQueryListProps<T>): T | undefined { | ||
if (Utils.isFunction(itemEqualsQuery)) { | ||
// .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 (const item of items) { | ||
if (itemEqualsQuery(item, query)) { | ||
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.
I would prefer the
const/type
approach so users can use string literals instead of importing this long-named type in order toswitch
on it.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.
@giladgray Product teams have requested string-literal enums in the past, but happy to do a simpler type. Is this what you mean?
Or perhaps just inline it?
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.
I like the first one,
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.
@adidahiya @giladgray Done