[Labs] InputList & Select components #1159

giladgray · 2017-05-30T19:27:26Z

Changes proposed in this pull request:

📦 new @blueprintjs/labs package for unstable and in-development components
InputList component composes input to filter list of items.
Select uses InputList to build basic dropdown chooser functionality.

Reviewers should focus on:

API design

Screenshot

blueprint-bot · 2017-05-30T19:40:25Z

scaffold tests, disable coverage check in labs package

Preview: documentation
Coverage: core | datetime

adidahiya · 2017-05-30T19:35:35Z

gulp/util/karma-config.js

@@ -3,6 +3,15 @@
 */
 "use strict";

+const COVERAGE_PERCENT = 80;
+const COVERAGE_PERCENT_MAX = 90;
+const COVERAGE_CHECK = {


this is only used once, I don't think you need to make a const

adidahiya · 2017-05-30T19:35:40Z

packages/docs/src/common/propsStore.ts


    public getInterface = (name: string) => {
-        return this.props[name];
+        // remove generics from end of name
+        const actualName = /^(\w+)<?/.exec(name)[1];


hmm this feels like it should probably be fixed at the ts-quick-docs layer? interfaces have names and type params, which we could preserve in a data object and avoid this regex

agreed but needed quick fix

filed giladgray/ts-quick-docs#25 to track

adidahiya · 2017-05-30T19:35:59Z

gulp/util/karma-config.js

@@ -3,6 +3,15 @@
 */
 "use strict";

+const COVERAGE_PERCENT = 80;
+const COVERAGE_PERCENT_MAX = 90;


"max" doesn't sound right semantically; what does istanbul call these numbers?

adidahiya · 2017-05-30T19:37:43Z

packages/labs/src/inputList.tsx

+ * and https://github.com/palantir/blueprint/blob/master/PATENTS
+ */
+
+// import * as classNames from "classnames";


commented code

adidahiya · 2017-05-30T19:39:07Z

packages/labs/src/inputList.tsx

+
+import { IProps, Keys, Utils } from "@blueprintjs/core";
+
+export interface ICoreInputListProps<D> extends IProps {


I think this type param should be T... D probably isn't semantically clear enough to warrant a deviation from the naming standard (same for all the other interfaces)

adidahiya · 2017-05-30T19:44:14Z

packages/labs/src/inputList.tsx

+     * Customize filtering of entire items array. Return subset of items that pass filter.
+     * (Some filter algorithms operate on the entire set, rather than individual items.)
+     */
+    itemsFilterer?: (items: D[], query: string) => D[];


w/ above suggestion: itemListPredicate

adidahiya · 2017-05-30T19:46:04Z

packages/labs/src/inputList.tsx

+export class InputList<D> extends React.Component<IInputListProps<D>, IInputListState<D>> {
+    public static displayName = "Blueprint.InputList";
+
+    public static ofType<T>() { return InputList as new () => InputList<T>; }


if it has a return keyword might as well make it multiline...

adidahiya · 2017-05-30T19:46:54Z

packages/labs/src/inputList.tsx

+     */
+    private shouldCheckActiveItemInViewport: boolean;
+
+    constructor(props?: IInputListProps<D>, context?: any) {


props isn't optional

adidahiya · 2017-05-30T19:48:37Z

packages/labs/src/select.tsx

+    inputProps?: IInputGroupProps & HTMLInputProps;
+
+    /** React props to spread to `Popover`. Note that `content` cannot be changed. */
+    popoverProps?: Partial<IPopoverProps>;


need Partial<IPopoverProps> & object; currently this type is too permissive

adidahiya · 2017-05-30T19:50:20Z

packages/labs/src/select.tsx

+                leftIconName="search"
+                onChange={onQueryChange}
+                placeholder="Filter..."
+                rightElement={<InputClearButton onClick={this.clearQuery} visible={query.length > 0} />}


meh, this doesn't seem better than inlining the ternary... {query.length > 0 ? <Button .../> : undefined}

it fits on one line and doesn't require a const. analogous to renderItems member below; is that pattern preferable?

yeah, I'd prefer to encapsulate it inside this class as a private method. or just an inline expression in this method. const InputClearButton is only used once, so it doesn't make much sense as a separate component

cmslewis · 2017-05-30T20:59:50Z

packages/labs/README.md

+Current components:
+
+- `InputList` is a low-level component for composing an `InputGroup` to filter an array of `items`.
+- `Select` is a high-level component for choosing items for a list. It can be used instead of the


for choosing items from a list?

cmslewis · 2017-05-30T21:01:12Z

packages/labs/examples/data.ts

+    { title: "Inglourious Basterds", year: 2009 },
+    { title: "Snatch", year: 2000 },
+    { title: "3 Idiots", year: 2009 },
+    { title: "Monty Python and the Holy Grail", year: 1975 },


Would it make sense to select then F5 in VS Code to alphabetize these? Would placate my inner demon.

no they're actually ordered, top 100 movies from imdb. i should note in code.

renamed const accordingly: TOP_100_FILMS

Sounds good.

(Resolved)

…s with generics

blueprint-bot · 2017-05-30T22:32:30Z

refactor clear button SFC

Preview: documentation | table
Coverage: core | datetime

cmslewis · 2017-05-31T21:24:19Z

FYI there's some z-index weirdness on the docs page:

Looks like the navbar border is above the popover, but the popover is above the navigator search box.

cmslewis · 2017-05-31T21:27:47Z

Couple UX questions:

I'd expect to be able to type "10." and pull up the result prefixed with "10," but that doesn't happen at present.
Pressing enter to select an item in Select, doesn't close the popover. But I see an explicit this.setState({ isOpen: false }) in the code.

ICoreInputListProps => IListItemsProps cuz it’s all about a list of items. Remove `onQueryChange` because it’s a renderer implementation detail. Rename renderer props to `handle*` to indicate they are handler callbacks, not events.

Pull activeItem state up to Select; InputList requires controlled props for it now (and translates item to index).

…repaint Super awesome for scroll adjustments + no flicker!

giladgray · 2017-06-06T18:53:36Z

resolved all issues by @cmslewis above.

the z-index weirdness was due to top margin on the input group causing the top 5px of popover to have transparent background, so navbar border would show through it 😱 . i had to wiggle some styles to get the padding to work correctly.

blueprint-bot · 2017-06-06T18:58:47Z

major docs updates, fix navbar/border bleed issue

Preview: documentation | table
Coverage: core | datetime

adidahiya · 2017-06-06T19:12:23Z

Looking great so far!

One minor UX request: can we get ESC to close the dropdown when it's open & focused?

adidahiya · 2017-06-06T19:28:45Z

packages/labs/src/inputList.tsx

+     * Selection handler that should be invoked when a new item has been chosen,
+     * perhaps because the user clicked it.
+     */
+    handleItemSelect: (item: T, event: React.SyntheticEvent<HTMLElement>) => void;


generally, handlers in props interfaces should be named onItemSelect. is there a reason why this is exempt?

this is not an event listener prop for you to supply but an actual handler function, passed to the renderer callback prop:

// very pseudocode ahead <InputList renderer=({ handleItemSelect }) => <div onClick={handleItemSelect} />) />

see the docs note on this interface:

An object with the following properties will be passed to an InputList renderer.

adidahiya · 2017-06-06T19:30:37Z

packages/labs/src/select.tsx

+
+    public state: ISelectState<T> = { isOpen: false, query: "" };
+
+    private DInputList = InputList.ofType<T>();


missed a rename? StaticInputList maybe? ConcreteInputList?

oh TInputList. this is neither static nor concrete.

llorca · 2017-06-06T19:56:23Z

One minor UX request: can we get ESC to close the dropdown when it's open & focused?

Can we make that a Popover feature as opposed to restricting it in Select? Useful in so many places

giladgray · 2017-06-06T22:13:35Z

esc key is already supported, and pressing it closes the popover. what is missing? also, popoverProps={{ canEscapeKeyClose: false }} is already a thing.

cmslewis · 2017-06-06T22:34:39Z

Text-overflow issue in this particular Select example:
Animation on the minimal style feels like it could use some scaleing or something. Feels mildly incomplete right now.
If filterable={false}, the top padding is excessive because of the additional 5px top padding on pt-menu; I'd expect it to be 5px all around.
Popover doesn't move with the input field when the page is resized.

giladgray · 2017-06-06T22:38:04Z

@cmslewis:

text wrapping solved by [Menu] Flexify menu items #1136
not relevant to this PR; minimal animation intentionally has no scale.
known, fixed locally
fuck.

giladgray · 2017-06-06T22:40:35Z

above is also an issue on PopoverExample so def not a regression here. that's part of the larger issue with popovers not staying where they should be.

llorca · 2017-06-06T22:40:58Z

+1 on the filterable={false} padding problem. The other things aren't related to Select nor InputList.

I also saw a layout issue:

cmslewis · 2017-06-06T22:40:39Z

packages/labs/examples/data.ts

+    { title: "Inglourious Basterds", year: 2009 },
+    { title: "Snatch", year: 2000 },
+    { title: "3 Idiots", year: 2009 },
+    { title: "Monty Python and the Holy Grail", year: 1975 },


(Resolved)

cmslewis · 2017-06-06T22:41:15Z

packages/labs/examples/selectExample.tsx

+import { Select } from "../src";
+import { Film, TOP_100_FILMS } from "./data";
+
+const MovieSelect = Select.ofType<Film>();


nit: FilmSelect for consistency of terminology?

cmslewis · 2017-06-06T22:43:39Z

packages/labs/src/blueprint-labs.scss

+ * and https://github.com/palantir/blueprint/blob/master/PATENTS
+ */
+
+


nit (lintable?): delete double newline?

cmslewis · 2017-06-06T22:45:58Z

packages/labs/src/inputList.tsx

+        if (nextProps.items !== this.props.items
+            || nextProps.itemListPredicate !== this.props.itemListPredicate
+            || nextProps.itemPredicate !== this.props.itemPredicate
+            || nextProps.query !== this.props.query


Let's use Utils.shallowCompareKeys eventually, or now if you already have it on this branch.

that function is only in table right? don't want to make that refactor now...

Good point. 😛

cmslewis · 2017-06-06T22:48:37Z

packages/labs/src/inputList.tsx

+
+function getFilteredItems<T>({ items, itemPredicate, itemListPredicate, query }: IInputListProps<T>) {
+    if (Utils.isFunction(itemListPredicate)) {
+        // TODO: this can reorder the items, which can cause weirdness while filtering. do some design.


TODO in this PR?

these are questions for the future

turns out this one is fine--the reordering is useful and the component handles it well.

cmslewis · 2017-06-06T22:51:40Z

packages/labs/src/inputList.tsx

+    /**
+     * Customize filtering of entire items array. Return subset of items that pass filter.
+     * (Some filter algorithms operate on the entire set, rather than individual items.)
+     */


Can we clarify how these two props (itemPredicate and itemListPredicate) work together if they're both defined? In that case, is an error thrown, is one of these props ignored, etc. (I see from the code that itemListPredicate takes precedence.)

there is a section in the docs

I think that it would be nice if we can provide a warning that says something along the lines of "you've provided both itemListPredicate and itemPredicate, itemPredicate will be ignored" or something.

cmslewis · 2017-06-06T22:53:02Z

packages/labs/src/select.tsx

+
+    public render() {
+        // omit props specific to this component, spread the rest.
+        // TODO: should InputList just support arbitrary props? could be useful for re-rendering


TODO in this PR? if not, can we file an issue and add a link here?

what do you think about the question? should it just support [key: string]: any; so you can spread arbitrary things?

cmslewis · 2017-06-06T22:56:38Z

packages/labs/src/select.tsx

+    private renderItems({ activeItem, filteredItems, handleItemSelect }: IInputListRendererProps<T>) {
+        const { itemRenderer, noResults } = this.props;
+        return filteredItems.length > 0
+            ? filteredItems.map((item) => itemRenderer(item, item === activeItem, (e) => handleItemSelect(item, e)))


Is it up to the consumer to add a key to each item? Should we mention that in the docs?

i can add a note in prop docs. but also React will tell you.

adidahiya · 2017-06-06T23:59:32Z

feedback here seems pretty minor; let's get a rough cut of this out and fixup small things later

suchanlee

looks awesome!

suchanlee · 2017-06-07T04:22:59Z

packages/labs/README.md

+- `InputList` is a low-level component for composing an `InputGroup` to filter an array of `items`.
+- `Select` is a high-level component for choosing items from a list. It can be used instead of the
+  HTML `<select>` element.
+- `Suggest` is a high-level component for an `InputGroup` that suggests completions based on its


Is this not part of this PR?

Not yet, we're building that next

suchanlee · 2017-06-07T04:23:56Z

packages/labs/examples/selectExample.tsx

+                onItemSelect={this.handleValueChange}
+                popoverProps={{ popoverClassName: minimal ? Classes.MINIMAL : "" }}
+            >
+                <Button text={film ? film.title : "(No selection)"} rightIconName="double-caret-vertical" />


nit: recommend 1 prop per line if there are > 1 props, for better diffing

i do it based on line length typically but i hear that

suchanlee · 2017-06-07T04:28:08Z

packages/labs/package.json

+  "style": "dist/blueprint-labs.css",
+  "unpkg": "dist/labs.bundle.js",
+  "dependencies": {
+    "@blueprintjs/core": "^1.18.0",


(not knowing how blueprint dependencies are normally packaged) Would it be better to keep blueprintjs/core as peerDependency so that there aren't version conflicts with applications that already consume blueprint, but at a different version?

nope, peer dependencies are annoying in general and should be avoided. there's no harm to having multiple versions of blueprint in your node_modules tree, and everyone should be setting semver ranges appropriately so that they rarely get into that situation anyway. since we ship a single global CSS file, it's pretty hard to run into conflicts within a single app (your bundler will only pick up node_modules/@blueprinjs/core/dist/blueprint.css)

suchanlee · 2017-06-07T04:35:32Z

packages/labs/src/inputList.tsx

+    /**
+     * Customize filtering of entire items array. Return subset of items that pass filter.
+     * (Some filter algorithms operate on the entire set, rather than individual items.)
+     */


I think that it would be nice if we can provide a warning that says something along the lines of "you've provided both itemListPredicate and itemPredicate, itemPredicate will be ignored" or something.

suchanlee · 2017-06-07T04:44:39Z

packages/labs/src/inputList.tsx

+     * The active item is the current keyboard-focused element.
+     * Listen to `onActiveItemChange` for updates from interactions.
+     */
+    activeItem: T;


should this be T | undefined, since if filtered items = [], there would be no active item? same for onActiveItemChange below

suchanlee · 2017-06-07T05:10:49Z

packages/labs/src/inputList.tsx

+
+    private getActiveIndex() {
+        // NOTE: this operation is O(n) so it should be avoided in render(). safe for events though.
+        return this.state.filteredItems.indexOf(this.props.activeItem);


nit: this function gets called multiple times every time the user presses up/down and may lead to a laggy experience if the filteredItems size is large and the user holds up/down. May be able to make this faster with something like an ordered map but probably not worth the trouble if there are plans to virtualize this. Actually, if the number of items is large, the rendering perf may dwarf whatever perf issue this may result in...

worth consideration: a simple @debounce

suchanlee · 2017-06-07T05:20:29Z

packages/labs/src/select.tsx

+    itemRenderer: (item: T, isActive: boolean, onClick: React.MouseEventHandler<HTMLElement>) => JSX.Element;
+
+    /** React child to render when filtering items returns zero results. */
+    noResults?: string | JSX.Element;


thoughts on changing this to nonIdealState? I think that this could be used for displaying a spinner if the user decides to use server-side search/pagination on input change

Let's keep it as is for now, some apps have complex empty states that I need to review again to make sure NonIdealState is always appropriate. (I suppose the current JSX.Element already allows you to provide your own NonIdealState 👍)

using an actual NonIdealState in the menu might be too much UI for a small space? i prefer this name, as the semantics are clear but what you put there is up to you.

an actual NonIdealState is definitely too big for this, but can totally see it being used for a "non ideal state". I think the name noResults might be a bit limiting but up to you

but this prop is specifically rendered when there are no results: empty items array or query returns empty array (same thing really).

hmm i suppose you're right

Oh I misunderstood, I thought you were talking about the typing. Up to you guys on the prop naming, we can always change it later

suchanlee · 2017-06-07T05:22:18Z

packages/labs/src/select.tsx

+
+export interface ISelectState<T> {
+    activeItem?: T;
+    isOpen?: boolean;


isOpen and query are never undefined. Should they be optional? or is it the blueprint's style to make all state fields optional?

blueprint style cuz we use old react typings (pre-Pick)

suchanlee · 2017-06-07T05:28:25Z

packages/labs/src/select.tsx

+        // not using defaultProps cuz they're hard to type with generics (can't use <T> on static members)
+        const { filterable = true, inputProps = {}, popoverProps = {} } = this.props;
+        const { handleKeyDown, handleKeyUp, query } = listProps;
+


nit: unnecessary newline?

suchanlee · 2017-06-07T05:43:31Z

packages/labs/src/inputList.tsx

+        }
+    }
+
+    public scrollActiveItemIntoView() {


seems like a good candidate for taking out into a reusable function/component

i mean it is in a reusable component... InputList doesn't do much else beyond keyboard selection

suchanlee · 2017-06-07T05:51:01Z

Any thoughts about supporting debouncing the list render for when the query changes? re-rendering a lot of items, for every query change can be expensive and can lead to a laggy experience

suchanlee · 2017-06-07T05:55:03Z

packages/labs/src/select.tsx

+     * this item is active (selected by keyboard arrows) and an `onClick` event handler that
+     * should be attached to the returned element.
+     */
+    itemRenderer: (item: T, isActive: boolean, onClick: React.MouseEventHandler<HTMLElement>) => JSX.Element;


thoughts on including the item index as well? Can support use cases like rendering N items

might as well move to object argument like renderer: (props) => JSX.Element

IMO that would work better if you include the index

cmslewis

Looks good for a first pass! We can keep working on things of course. <3 Labs.

similar docs too

blueprint-bot · 2017-06-07T23:49:26Z

fix tests

Preview: documentation | table
Coverage: core | datetime

Gilad Gray added 5 commits May 30, 2017 12:03

labs package setup

5c99ab6

add empty docs and scss files

a7fdbc7

⭐ InputList and Select components

6ece39d

remove generics from end of interface name in propsStore lookup

7a83107

scaffold tests, disable coverage check in labs package

3a1fa01

adidahiya reviewed May 30, 2017

View reviewed changes

cmslewis reviewed May 30, 2017

View reviewed changes

Gilad Gray added 6 commits May 30, 2017 15:04

Movie => Film

af50a6b

karma constants

df5eeb9

itemFilterer => itemPredicate

069fdcd

refactor to avoid constructor and defaultProps cuz they cause problem…

3564086

…s with generics

D => T in generics

720b40f

refactor clear button SFC

cbd5c01

Gilad Gray added 4 commits May 30, 2017 16:17

basic tests for both components

d6cdbb6

filterable switch in example

ea57167

Select controls popover isOpen

ea356a2

InputList public scrolling method, docs, test refactors

6c851d1

Gilad Gray added 3 commits May 31, 2017 15:45

Select extends AbstractComponent for timeout

a735f99

Select controls query so you don't have to

fd71619

Major refactors to interfaces & prop naming

5ccb058

ICoreInputListProps => IListItemsProps cuz it’s all about a list of items. Remove `onQueryChange` because it’s a renderer implementation detail. Rename renderer props to `handle*` to indicate they are handler callbacks, not events.

giladgray mentioned this pull request Jun 2, 2017

[Popover] Popover closes erroneously when given a react-select child #935

Closed

Gilad Gray added 5 commits June 1, 2017 19:24

More docs

23dac58

re-invoke popover lifecycle methods

6f2d925

Fix tests

5d6f302

Select resetOnSelect prop

2da1e09

Pull activeItem state up to Select; InputList requires controlled props for it now (and translates item to index).

Use requestAnimationFrame instead of setTimeout so it happens before …

7f4706f

…repaint Super awesome for scroll adjustments + no flicker!

major docs updates, fix navbar/border bleed issue

00257cc

adidahiya reviewed Jun 6, 2017

View reviewed changes

cmslewis reviewed Jun 6, 2017

View reviewed changes

adidahiya approved these changes Jun 6, 2017

View reviewed changes

suchanlee reviewed Jun 7, 2017

View reviewed changes

cmslewis approved these changes Jun 7, 2017

View reviewed changes

llorca approved these changes Jun 7, 2017

View reviewed changes

suchanlee approved these changes Jun 7, 2017

View reviewed changes

Gilad Gray added 5 commits June 7, 2017 15:55

Docs polish ⭐

757ad24

Reorder predicate args, activeItem is T | undefined

5c52c54

Select itemRenderer takes one object arg, like InputList renderer

0908b2d

similar docs too

add key to itemRenderer docs

356173b

fix tests

b1acaa8

giladgray merged commit ce6e6f9 into master Jun 8, 2017

giladgray deleted the gg/labs branch June 8, 2017 00:32

This was referenced Jun 8, 2017

fix wee dev bugs #1203

Merged

Prepare Release 1.19.0 #1211

Merged


		import { IProps, Keys, Utils } from "@blueprintjs/core";

		export interface ICoreInputListProps<D> extends IProps {


		public state: ISelectState<T> = { isOpen: false, query: "" };

		private DInputList = InputList.ofType<T>();

		* and https://github.com/palantir/blueprint/blob/master/PATENTS
		*/

[Labs] InputList & Select components #1159

[Labs] InputList & Select components #1159

Conversation

giladgray commented May 30, 2017 • edited Loading

Changes proposed in this pull request:

Reviewers should focus on:

Screenshot

blueprint-bot commented May 30, 2017

scaffold tests, disable coverage check in labs package

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

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

giladgray May 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented May 30, 2017

refactor clear button SFC

cmslewis commented May 31, 2017 • edited by giladgray Loading

cmslewis commented May 31, 2017 • edited by giladgray Loading

giladgray commented Jun 6, 2017

blueprint-bot commented Jun 6, 2017

major docs updates, fix navbar/border bleed issue

adidahiya commented Jun 6, 2017

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

llorca commented Jun 6, 2017

giladgray commented Jun 6, 2017

cmslewis commented Jun 6, 2017 • edited by giladgray Loading

giladgray commented Jun 6, 2017

giladgray commented Jun 6, 2017

llorca commented Jun 6, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adidahiya commented Jun 6, 2017

suchanlee left a 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

adidahiya Jun 7, 2017 • 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

giladgray commented May 30, 2017 •

edited

Loading

giladgray May 30, 2017 •

edited

Loading

cmslewis commented May 31, 2017 •

edited by giladgray

Loading

cmslewis commented May 31, 2017 •

edited by giladgray

Loading

cmslewis commented Jun 6, 2017 •

edited by giladgray

Loading

adidahiya Jun 7, 2017 •

edited

Loading