[Select] Create-able Select components #3306

shuyangli · 2019-01-21T20:59:59Z

Fixes #1710

Includes tests
Update documentation

Changes proposed in this pull request:

Select components, via IListItemsProps, now takes two additional props:
- createNewItemFromQuery?: (query: string) => T which converts a query to a new item; if this prop is present, the select component is considered "create-able";
- createNewItemRenderer?: (query: string, handleClick: React.MouseEventHandler<HTMLElement>) => JSX.Element | undefined which could be used to render a custom MenuItem at the end of the list, that represents "create a new element from query".
If these props are provided, when the component has a non-empty input, a "create new item" entry is rendered at the end of the filtered item list using the given createNewItemRenderer, and the user can activate it to create a new item with the current query.
If the component is create-able, when a query returns no results, pressing "enter" key will create a new item with the current query.

shuyangli · 2019-01-22T13:56:28Z

FLUPs:

integrating this with arrow keys
code sample in select-component.md that demonstrates using both new props
in selectComponentSuite.tsx tests, for renders create item if filtering returns empty list, use the real item list

shuyangli · 2019-02-06T12:26:21Z

@giladgray can you give some feedback on the API before I start with tests and documentation?

adidahiya · 2019-02-15T01:15:20Z

packages/select/src/common/listItemsProps.ts

+     * This is invoked when user interaction causes a new item to be created, either by pressing the `enter` key or
+     * by clicking on the "Create Item" option. It transforms a query string into an item type.
+     */
+    createItemFromQuery?: (query: string) => T;


API mostly looks good but can we be a little more explicit and say createNewItemFromQuery / createNewItemRenderer ?

adidahiya · 2019-02-15T01:15:32Z

packages/select/src/common/listItemsProps.ts

+     */
+    createItemRenderer?: (
+        query: string,
+        active: boolean,


when is this active param useful?

This is for when the "create" item should be highlighted; for example, if you interact with the dropdown with the up/down key, when your selection is on the create item, active would be true, and the item should be rendered accordingly

what else could you select in this case? there would be no other results to select, right? so it should be automatically "active"? I think the param is probably required for full customization of rendering, I'm not rejecting it, but maybe you could post a gif of that interaction?

The create item is actually always rendered as long as there's input, so the "active" flag is useful:

And to the point of "full customization of rendering" - the consumers provide code for rendering the "create" item just like they provide code for rendering other items, so to some extent this rendering is always custom

Should this be isActive, or is active the norm (like disabled)?

shuyangli · 2019-02-20T19:30:41Z

@adidahiya cleaned up tests and docs, this is ready for another look/merging

blueprint-bot · 2019-02-20T20:21:07Z

Merge branch 'develop' into sl/1710-create-able-multiselect

Previews: documentation | landing | table

cmslewis

@shuyangli Great stuff - I want this desperately! 🎉 Left some thoughts.

Also, can we add an "Allow creating new items" toggle to the Suggest example?

cmslewis · 2019-02-21T22:13:35Z

packages/docs-app/src/examples/select-examples/multiSelectExample.tsx

@@ -56,6 +59,8 @@ export class MultiSelectExample extends React.PureComponent<IExampleProps, IMult
            // explicit undefined (not null) for default behavior (show full list)
            undefined
        );
+        const maybeCreateNewItemFromQuery = this.state.allowCreate ? createFilm : undefined;
+        const maybeCreateNewItemRenderer = this.state.allowCreate ? this.renderCreateFilmOption : null;


nit; Unpack this on L50 since you're accessing it twice.

const { allowCreate, films, hasInitialContent, tagMinimal, popoverMinimal, ...flags } = this.state;

cmslewis · 2019-02-21T22:13:58Z

packages/docs-app/src/examples/select-examples/multiSelectExample.tsx

@@ -72,6 +77,8 @@ export class MultiSelectExample extends React.PureComponent<IExampleProps, IMult
                    tagRenderer={this.renderTag}
                    tagInputProps={{ tagProps: getTagProps, onRemove: this.handleTagRemove, rightElement: clearButton }}
                    selectedItems={this.state.films}
+                    createNewItemFromQuery={maybeCreateNewItemFromQuery}
+                    createNewItemRenderer={maybeCreateNewItemRenderer}


nit: Alphabetize props.

cmslewis · 2019-02-21T22:15:00Z

packages/docs-app/src/examples/select-examples/selectExample.tsx

@@ -55,12 +58,16 @@ export class SelectExample extends React.PureComponent<IExampleProps, ISelectExa
        ) : (
            undefined
        );
+        const maybeCreateNewItemFromQuery = this.state.allowCreate ? createFilm : undefined;
+        const maybeCreateNewItemRenderer = this.state.allowCreate ? this.renderCreateFilmOption : null;


cmslewis · 2019-02-21T22:16:41Z

packages/select/src/common/itemListRenderer.ts

+}
+
+export function getActiveItem<T>(activeItem: IQueryListActiveItem<T> | null | undefined): T | null {
+    if (activeItem && activeItem.type === QueryListActiveItemType.ITEM) {


Prefer explicit boolean expressions for clarity and to generally avoid possibly unintended ignoring of falsey values (like "", []). activeItem != null.

Also, I'd just use a ternary here.

return activeItem != null && activeItem.type === QueryListActiveItemType.ITEM ? activeItem.item : null;

cmslewis · 2019-02-21T22:19:12Z

packages/select/src/common/listItemsProps.ts

@@ -28,7 +28,7 @@ export interface IListItemsProps<T> extends IProps {
     * uncontrolled (managed by the component's state). Use `onActiveItemChange`
     * to listen for updates.
     */
-    activeItem?: T | null;
+    activeItem?: IQueryListActiveItem<T> | null;


This is a breaking change, no? Also, I don't love this API. Is there a way for us to just accept T | null, or T | IQueryListCreateItem<T> | null, or something?

I agree, this is a break and we can't merge as-is. I don't see a way to make this non-breaking if we try to adjust the existing prop types (we can make the addition to activeItem non-breaking but not onActiveItemChange)... so we'll have to add new optional props for this behavior. we could consider refactoring them to make the API more streamlined in the next major version, but for now I propose:

isCreateNewItemActive: boolean; onCreateNewItemActiveChange: () => null;

users will have to reconcile for themselves what happens between onActiveItemChange and onCreateNewItemActiveChange

cmslewis · 2019-02-21T22:31:48Z

packages/select/src/components/query-list/queryList.tsx

    private getActiveElement() {
        if (this.itemsParentRef != null) {
-            return this.itemsParentRef.children.item(this.getActiveIndex()) as HTMLElement;
+            if (this.state.activeItem && this.state.activeItem.type === QueryListActiveItemType.CREATE) {


Unpack

Explicit boolean expression

cmslewis · 2019-02-21T22:35:33Z

packages/select/src/components/query-list/queryList.tsx

-    private getNextActiveItem(direction: number, startIndex = this.getActiveIndex()): T | null {
+    private getNextActiveItem(direction: number, startIndex = this.getActiveIndex()): IQueryListActiveItem<T> | null {
+        if (this.isCreateItemRendered()) {
+            const reachedCreate =


Improve this boolean variable name. Maybe willSelectCreateItem?

cmslewis · 2019-02-21T22:35:56Z

packages/select/src/components/query-list/queryList.tsx

@@ -341,6 +398,10 @@ export class QueryList<T> extends React.Component<IQueryListProps<T>, IQueryList
        }
        Utils.safeInvoke(this.props.onActiveItemChange, activeItem);
    }
+
+    private isCreateItemRendered(): boolean {
+        return !!(this.props.createNewItemFromQuery && this.props.createNewItemRenderer && this.state.query !== "");


Use explicit boolean expressions for existence checks.

Implement validateProps to assert (in dev mode at least) that if one of these is provided, both must be provided. Otherwise the create item will just mysteriously never appear.

Add unit tests verifying the above.

cmslewis · 2019-02-21T22:37:47Z

packages/select/src/components/select/select-component.md

+            text={`Create "${query}"`}
+            active={active}
+            onClick={handleClick}
+            shouldDismissPopover={false}


Might as well alphabetize these IMO.

cmslewis · 2019-02-21T22:38:03Z

packages/select/src/components/select/select-component.md

+        itemPredicate={Films.itemPredicate}
+        itemRenderer={Films.itemRenderer}
+        noResults={<MenuItem disabled={true} text="No results." />}
+        createNewItemFromQuery={createFilm}


adidahiya · 2019-02-23T20:38:50Z

packages/select/src/common/listItemsProps.ts

@@ -28,7 +28,7 @@ export interface IListItemsProps<T> extends IProps {
     * uncontrolled (managed by the component's state). Use `onActiveItemChange`
     * to listen for updates.
     */
-    activeItem?: T | null;
+    activeItem?: IQueryListActiveItem<T> | null;


I agree, this is a break and we can't merge as-is. I don't see a way to make this non-breaking if we try to adjust the existing prop types (we can make the addition to activeItem non-breaking but not onActiveItemChange)... so we'll have to add new optional props for this behavior. we could consider refactoring them to make the API more streamlined in the next major version, but for now I propose:

isCreateNewItemActive: boolean; onCreateNewItemActiveChange: () => null;

users will have to reconcile for themselves what happens between onActiveItemChange and onCreateNewItemActiveChange

cmslewis · 2019-02-27T00:10:21Z

@adidahiya @shuyangli See my second attempt in this separate PR: #3381. It avoids breaking the API, and it also avoids bloating the props interface with any other fields.

shuyangli · 2019-02-28T00:48:32Z

Closing in favor of #3381 for backcompat-preserving API

Shuyang Li added 5 commits January 21, 2019 12:40

Add option to create new item

6668d07

Properly clear input on item click

3fcd343

Move props to queryList

b42418f

Lint and test

2548a01

Lint and docs

bccb5e2

shuyangli mentioned this pull request Jan 21, 2019

Create-able Select components #3304

Closed

2 tasks

shuyangli added 3 commits February 6, 2019 20:11

Properly handle keyboard events with create item

684919c

Merge branch 'develop' into sl/1710-create-able-multiselect

ff20ae6

Minor cleanup

6964bbb

Factor out getActiveItem

4596f78

adidahiya self-assigned this Feb 15, 2019

adidahiya reviewed Feb 15, 2019

View reviewed changes

adidahiya added the Status: waiting on author label Feb 15, 2019

Shuyang Li added 6 commits February 20, 2019 13:13

createItem => createNewItem

01a1538

Fix existing tests

d99d570

Use real item list in filtering test

6b676d5

More type-safe active item getter

2aa4b54

Add tests for interacting with create item

321cd03

Add docs for creating items

4c47f03

shuyangli removed the Status: waiting on author label Feb 20, 2019

Merge branch 'develop' into sl/1710-create-able-multiselect

59a86aa

palantir deleted a comment from blueprint-bot Feb 20, 2019

shuyangli changed the title ~~Create-able Select components~~ [Select] Create-able Select components Feb 20, 2019

palantir deleted a comment from blueprint-bot Feb 20, 2019

cmslewis reviewed Feb 21, 2019

View reviewed changes

adidahiya suggested changes Feb 23, 2019

View reviewed changes

cmslewis mentioned this pull request Feb 27, 2019

[Select] Create-able Select components (a different approach) #3381

Merged

2 tasks

shuyangli closed this Feb 28, 2019

shuyangli deleted the sl/1710-create-able-multiselect branch February 28, 2019 00:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Select] Create-able Select components #3306

[Select] Create-able Select components #3306

shuyangli commented Jan 21, 2019 •

edited

Loading

shuyangli commented Jan 22, 2019 •

edited

Loading

shuyangli commented Feb 6, 2019

adidahiya Feb 15, 2019

shuyangli Feb 20, 2019

adidahiya Feb 15, 2019

shuyangli Feb 20, 2019

adidahiya Feb 20, 2019

shuyangli Feb 20, 2019

shuyangli Feb 20, 2019

cmslewis Feb 21, 2019

shuyangli commented Feb 20, 2019

blueprint-bot commented Feb 20, 2019

cmslewis left a comment •

edited

Loading

cmslewis Feb 21, 2019

cmslewis Feb 21, 2019

cmslewis Feb 21, 2019

cmslewis Feb 21, 2019

cmslewis Feb 21, 2019

adidahiya Feb 23, 2019

cmslewis Feb 21, 2019

cmslewis Feb 21, 2019

cmslewis Feb 21, 2019

cmslewis Feb 21, 2019

cmslewis Feb 21, 2019

adidahiya Feb 23, 2019

cmslewis commented Feb 27, 2019

shuyangli commented Feb 28, 2019

[Select] Create-able Select components #3306

[Select] Create-able Select components #3306

Conversation

shuyangli commented Jan 21, 2019 • edited Loading

Fixes #1710

Changes proposed in this pull request:

shuyangli commented Jan 22, 2019 • edited Loading

shuyangli commented Feb 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuyangli commented Feb 20, 2019

blueprint-bot commented Feb 20, 2019

Merge branch 'develop' into sl/1710-create-able-multiselect

cmslewis left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmslewis commented Feb 27, 2019

shuyangli commented Feb 28, 2019

shuyangli commented Jan 21, 2019 •

edited

Loading

shuyangli commented Jan 22, 2019 •

edited

Loading

cmslewis left a comment •

edited

Loading