Skip to content
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

[Select] Create-able Select components #3306

Closed
wants to merge 16 commits into from
8 changes: 8 additions & 0 deletions packages/docs-app/src/examples/select-examples/films.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -182,3 +182,11 @@ export const filmSelectProps = {
itemRenderer: renderFilm,
items: TOP_100_FILMS,
};

export function createFilm(title: string): IFilm {
return {
rank: 100 + Math.floor(Math.random() * 100 + 1),
title,
year: new Date().getFullYear(),
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import * as React from "react";
import { Button, H5, Intent, ITagProps, MenuItem, Switch } from "@blueprintjs/core";
import { Example, IExampleProps } from "@blueprintjs/docs-theme";
import { ItemRenderer, MultiSelect } from "@blueprintjs/select";
import { filmSelectProps, IFilm, TOP_100_FILMS } from "./films";
import { createFilm, filmSelectProps, IFilm, TOP_100_FILMS } from "./films";

const FilmMultiSelect = MultiSelect.ofType<IFilm>();

Expand All @@ -19,6 +19,7 @@ export interface IMultiSelectExampleState {
films: IFilm[];
hasInitialContent: boolean;
intent: boolean;
allowCreate: boolean;
openOnKeyDown: boolean;
popoverMinimal: boolean;
resetOnSelect: boolean;
Expand All @@ -27,6 +28,7 @@ export interface IMultiSelectExampleState {

export class MultiSelectExample extends React.PureComponent<IExampleProps, IMultiSelectExampleState> {
public state: IMultiSelectExampleState = {
allowCreate: false,
films: [],
hasInitialContent: false,
intent: false,
Expand All @@ -42,6 +44,7 @@ export class MultiSelectExample extends React.PureComponent<IExampleProps, IMult
private handleTagMinimalChange = this.handleSwitchChange("tagMinimal");
private handleIntentChange = this.handleSwitchChange("intent");
private handleInitialContentChange = this.handleSwitchChange("hasInitialContent");
private handleAllowCreateChange = this.handleSwitchChange("allowCreate");

public render() {
const { films, hasInitialContent, tagMinimal, popoverMinimal, ...flags } = this.state;
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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


const clearButton = films.length > 0 ? <Button icon="cross" minimal={true} onClick={this.handleClear} /> : null;

Expand All @@ -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}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Alphabetize props.

/>
</Example>
);
Expand All @@ -96,6 +103,11 @@ export class MultiSelectExample extends React.PureComponent<IExampleProps, IMult
checked={this.state.hasInitialContent}
onChange={this.handleInitialContentChange}
/>
<Switch
label="Allow creating new films"
checked={this.state.allowCreate}
onChange={this.handleAllowCreateChange}
/>
<H5>Tag props</H5>
<Switch
label="Minimal tag style"
Expand Down Expand Up @@ -137,6 +149,20 @@ export class MultiSelectExample extends React.PureComponent<IExampleProps, IMult
);
};

private renderCreateFilmOption = (
query: string,
active: boolean,
handleClick: React.MouseEventHandler<HTMLElement>,
) => (
<MenuItem
icon="add"
text={`Create "${query}"`}
active={active}
onClick={handleClick}
shouldDismissPopover={false}
/>
);

private handleTagRemove = (_tag: string, index: number) => {
this.deselectFilm(index);
};
Expand Down
28 changes: 27 additions & 1 deletion packages/docs-app/src/examples/select-examples/selectExample.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,12 @@ import * as React from "react";
import { Button, H5, MenuItem, Switch } from "@blueprintjs/core";
import { Example, IExampleProps } from "@blueprintjs/docs-theme";
import { Select } from "@blueprintjs/select";
import { filmSelectProps, IFilm, TOP_100_FILMS } from "./films";
import { createFilm, filmSelectProps, IFilm, TOP_100_FILMS } from "./films";

const FilmSelect = Select.ofType<IFilm>();

export interface ISelectExampleState {
allowCreate: boolean;
film: IFilm;
filterable: boolean;
hasInitialContent: boolean;
Expand All @@ -27,6 +28,7 @@ export interface ISelectExampleState {

export class SelectExample extends React.PureComponent<IExampleProps, ISelectExampleState> {
public state: ISelectExampleState = {
allowCreate: false,
disableItems: false,
disabled: false,
film: TOP_100_FILMS[0],
Expand All @@ -38,6 +40,7 @@ export class SelectExample extends React.PureComponent<IExampleProps, ISelectExa
resetOnSelect: false,
};

private handleAllowCreateChange = this.handleSwitchChange("allowCreate");
private handleDisabledChange = this.handleSwitchChange("disabled");
private handleFilterableChange = this.handleSwitchChange("filterable");
private handleInitialContentChange = this.handleSwitchChange("hasInitialContent");
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.


return (
<Example options={this.renderOptions()} {...this.props}>
<FilmSelect
{...filmSelectProps}
{...flags}
createNewItemFromQuery={maybeCreateNewItemFromQuery}
createNewItemRenderer={maybeCreateNewItemRenderer}
disabled={disabled}
itemDisabled={this.isItemDisabled}
initialContent={initialContent}
Expand Down Expand Up @@ -110,6 +117,11 @@ export class SelectExample extends React.PureComponent<IExampleProps, ISelectExa
checked={this.state.disableItems}
onChange={this.handleItemDisabledChange}
/>
<Switch
label="Allow creating new items"
checked={this.state.allowCreate}
onChange={this.handleAllowCreateChange}
/>
<H5>Popover props</H5>
<Switch
label="Minimal popover style"
Expand All @@ -120,6 +132,20 @@ export class SelectExample extends React.PureComponent<IExampleProps, ISelectExa
);
}

private renderCreateFilmOption = (
query: string,
active: boolean,
handleClick: React.MouseEventHandler<HTMLElement>,
) => (
<MenuItem
icon="add"
text={`Create "${query}"`}
active={active}
onClick={handleClick}
shouldDismissPopover={false}
/>
);

private handleValueChange = (film: IFilm) => this.setState({ film });

private handleSwitchChange(prop: keyof ISelectExampleState) {
Expand Down
19 changes: 18 additions & 1 deletion packages/select/src/common/itemListRenderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,23 @@
* Licensed under the terms of the LICENSE file distributed with this project.
*/

export enum QueryListActiveItemType {
CREATE = "create",
ITEM = "item",
}

export interface IQueryListActiveItem<T> {
type: QueryListActiveItemType;
item: T | null;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

return activeItem.item;
}
return null;
}

/**
* An object describing how to render the list of items.
* An `itemListRenderer` receives this object as its sole argument.
Expand All @@ -13,7 +30,7 @@ export interface IItemListRendererProps<T> {
* The currently focused item (for keyboard interactions), or `null` to
* indicate that no item is active.
*/
activeItem: T | null;
activeItem: IQueryListActiveItem<T> | null;

/**
* Array of items filtered by `itemListPredicate` or `itemPredicate`.
Expand Down
22 changes: 19 additions & 3 deletions packages/select/src/common/listItemsProps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/

import { IProps, Utils } from "@blueprintjs/core";
import { ItemListRenderer } from "./itemListRenderer";
import { IQueryListActiveItem, ItemListRenderer } from "./itemListRenderer";
import { ItemRenderer } from "./itemRenderer";
import { ItemListPredicate, ItemPredicate } from "./predicate";

Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/** Array of items in the list. */
items: T[];
Expand Down Expand Up @@ -111,7 +111,7 @@ export interface IListItemsProps<T> extends IProps {
* in the list, selecting an item makes it active, and changing the query may reset it to
* the first item in the list if it no longer matches the filter.
*/
onActiveItemChange?: (activeItem: T | null) => void;
onActiveItemChange?: (activeItem: IQueryListActiveItem<T> | null) => void;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.


/**
* Callback invoked when an item from the list is selected,
Expand All @@ -124,6 +124,22 @@ export interface IListItemsProps<T> extends IProps {
*/
onQueryChange?: (query: string, event?: React.ChangeEvent<HTMLInputElement>) => void;

/**
* If provided, allows new items to be created with the current query string.
* 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.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Suggested edits for clarity:

/**
 * If provided, allows new items to be created using the current query
 * string; it should transform the query string into a new item. Will be
 * invoked when the user selects the item returned from
 * `createNewItemRenderer` (either via `Enter` or click).
 */

Also, should we require createNewItemRenderer to be defined if this callback is defined (and vice versa)?

createNewItemFromQuery?: (query: string) => T;

/**
* Custom renderer to transform the current query string into a selectable "Create Item" option.
*/
createNewItemRenderer?: (
query: string,
active: boolean,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when is this active param useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

active-create

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

handleClick: React.MouseEventHandler<HTMLElement>,
) => JSX.Element | undefined;

/**
* Whether the active item should be reset to the first matching item _every
* time the query changes_ (via prop or by user input).
Expand Down
Loading