QueryList itemRenderer prop & renderItem method #1877

giladgray · 2017-12-09T02:09:01Z

Please read the new docs in select-component.md (including diffs in example code)!

Looking for feedback on these changes. I think this is mostly an improvement but I need validation. 😬
API breaks ahead and no new features (yet) but this refactor greatly reduces duplication and enables desirable improvements like disabled support (through modifiers).

Notable Changes

🤞 - denotes future / wishlist / proposals; cool stuff this feature enables down the road.

new ItemRenderer<T> type uses two arguments so the Props bag doesn't need to be generic. also allows for null return values. (eventually, 🤞 props bag can be stored in state and intelligently diffed, rather than reconstructed in every render.)
```
export type ItemRenderer<T> = (item: T, itemProps: IItemRendererProps) => JSX.Element | null;
```

QueryList IItemModifiers for data about how to render. this sets us up for disabled support [Labs] MultiSelect needs a disabled prop #1834, and maybe even 🤞 user-defined modifiers a la react-day-picker or Popper.js!

export interface IItemModifiers {
    /** Whether this is the "active" (focused) item, meaning keyboard interactions will act upon it. */
    active: boolean;

    /** Whether this item is disabled and should ignore interactions. */
    disabled: boolean;

    /** Whether this item matches the predicate. A typical renderer could hide `false` values. */
    matchesPredicate: boolean;
}

move itemRenderer prop definition to QueryList itself (because every component redefined it exactly).
add renderItem method in IQueryListRendererProps that retrieves modifiers and delegates to itemRenderer prop. makes for much simpler item rendering!
refactor examples to share a common renderer and predicate implementation, and rename the data file to films.

…e new signature

…ates to itemRenderer prop makes for much simpler item rendering!

…ist-modifiers

blueprint-bot · 2017-12-09T05:41:51Z

fix the tests

Preview: documentation

…ist-modifiers

blueprint-bot · 2017-12-15T00:55:42Z

Merge branch 'master' of github.com:palantir/blueprint into gg/querylist-modifiers

Preview: documentation

adidahiya

overall API breaks lgtm

adidahiya · 2017-12-15T01:28:38Z

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

        ) : (
            undefined
        );

        return (
            <FilmMultiSelect
+                {...Films}


this is pretty confusing / too clever. please refactor to export something like multiSelectProps from the "./films" module

then you also wouldn't have awkward looking code like type Film = Films.Film;

adidahiya · 2017-12-15T01:30:03Z

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

@@ -108,4 +113,27 @@ export const TOP_100_FILMS = [
    { title: "Monty Python and the Holy Grail", year: 1975 },
 ].map((m, index) => ({ ...m, rank: index + 1 }));

-export type Film = typeof TOP_100_FILMS[0];
+export type Film = typeof items[0];


would rather just make this an interface so its usage is more obvious in the other files.

interface IFilm { ... }

adidahiya · 2017-12-15T01:32:18Z

packages/select/src/components/query-list/itemRenderer.ts

+
+    // TODO: not happy with this name
+    /** Whether this item matches the predicate. A typical renderer could hide `false` values. */
+    filtered: boolean;


how about matchesFilter: boolean

sure, but matchesPredicate would be more accurate (itemPredicate etc), and now we're getting verbose.

sure, yeah, makes sense

hellochar

I like item totally separate from props in general

hellochar · 2017-12-15T01:33:01Z

packages/select/src/components/query-list/itemRenderer.ts

+ * An object describing how to render a particular item.
+ * An `itemRenderer` receives the item as its first argument, and this object as its second argument.
+ */
+export interface IItemRendererProps {


What's the benefit of splitting the props/modifier? Why is it no longer generic?

it was only generic for the item: T field -- everything else has well-defined types.
also item is the only user-supplied field; the others are derived from other props and state. so now the derived fields are in a separate place and can be reused across renders, rather than being forced to create a new object just to change item.

hellochar · 2017-12-15T01:33:45Z

packages/select/src/components/query-list/itemRenderer.ts

+
+    // TODO: not happy with this name
+    /** Whether this item matches the predicate. A typical renderer could hide `false` values. */
+    filtered: boolean;


Specifically having a "filtered" prop is a little confusing; why is filtering special vs any other thing that users might want to do

filtering is one of QueryList's existential features: itemPredicate / itemListPredicate.
this API sets us up to support arbitrary user-defined modifiers.

themadcreator

Naming comments

themadcreator · 2017-12-15T20:33:15Z

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

 /** Top 100 films as rated by IMDb users. http://www.imdb.com/chart/top */
-export const TOP_100_FILMS = [
+export const items = [


nit: why not UPPER_CASE?

themadcreator · 2017-12-15T20:36:11Z

packages/select/src/components/omnibar/omnibar.tsx

-                item,
-            }),
-        );
+    private renderItems({ items, renderItem }: IQueryListRendererProps<T>) {


not sure where IQueryListRendererProps is defined, but i think this should be itemRenderer

themadcreator · 2017-12-15T20:37:35Z

packages/select/src/components/query-list/itemRenderer.ts

+    /** Whether this item is disabled and should ignore interactions. */
+    disabled: boolean;
+
+    // TODO: not happy with this name


rename or not. remove comment.

themadcreator · 2017-12-15T20:38:10Z

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

+     * `QueryList` will retrieve modifiers for the item and delegate to `itemRenderer` prop for the actual rendering.
+     * The second parameter `index` is optional here; if provided, it will be passed through `itemRenderer` props.
+     */
+    renderItem: (item: T, index?: number) => JSX.Element;


itemRenderer

nope, this is a method given to you, not a prop you define.

themadcreator · 2017-12-15T20:39:10Z

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

@@ -107,6 +108,13 @@ export interface IQueryListRendererProps<T> extends IProps {
     */
    handleKeyUp: React.KeyboardEventHandler<HTMLElement>;


shouldn't this be onKeyUp for the prop? handle[Event] is for method names, right?

this is a method passed to the renderer. hard to tell without context above.

…list-modifiers

blueprint-bot · 2018-01-25T01:20:15Z

modifiers.filtered => modifiers.matchesPredicate

Preview: documentation | landing | table

giladgray · 2018-01-25T02:00:21Z

@adidahiya this should be ready to go as well!

adidahiya · 2018-01-25T16:48:47Z

packages/select/test/multiSelectTests.tsx

@@ -11,18 +11,16 @@ import * as React from "react";
 import * as sinon from "sinon";

 // this is an awkward import across the monorepo, but we'd rather not introduce a cyclical dependency or create another package


in a future PR, we should move this to packages/select/src/__fixtures__/. then you'd do the import from "@blueprintjs/select/src/__fixtures__/films"

btw we'd want to .npmignore __tests__ and __fixtures__

cool yes i like that

Gilad Gray added 11 commits November 28, 2017 12:50

QueryList IItemModifiers for data about _how_ to render

fae1b60

refactor examples, common Film impls

ad14468

revert to single object arg syntax cuz it's established and reliable

637311a

rename data => films.tsx

dd14407

fix MultiSelectExample

ea72c6b

focused => active

7aa16e3

update Select docs

a30442b

add itemRenderer.ts file with new ItemRenderer two-arg signature

ea63178

move itemRenderer prop definition to QueryList itself, refactor to us…

ad16691

…e new signature

renderItem method in IQueryListRendererProps gets modifiers and deleg…

6f6f209

…ates to itemRenderer prop makes for much simpler item rendering!

update Item Renderer API docs

701bddb

giladgray added Status: in discussion Type: breaking change Type: refactor labels Dec 9, 2017

Gilad Gray added 2 commits December 8, 2017 18:10

Merge branch 'master' of github.com:palantir/blueprint into gg/queryl…

8d26c5c

…ist-modifiers

fix the tests

2ae6860

Gilad Gray added 2 commits December 14, 2017 14:22

Merge branch 'master' of github.com:palantir/blueprint into gg/queryl…

c986e0b

…ist-modifiers

Merge branch 'master' of github.com:palantir/blueprint into gg/queryl…

87f0d73

…ist-modifiers

adidahiya suggested changes Dec 15, 2017

View reviewed changes

hellochar reviewed Dec 15, 2017

View reviewed changes

cmslewis changed the title ~~[Proposal] QueryList itemRenderer prop & renderItem method~~ QueryList itemRenderer prop & renderItem method Dec 15, 2017

cmslewis requested a review from themadcreator December 15, 2017 19:15

themadcreator removed the Status: in discussion label Dec 15, 2017

themadcreator reviewed Dec 15, 2017

View reviewed changes

adidahiya changed the base branch from master to develop January 15, 2018 17:15

Gilad Gray added 3 commits January 24, 2018 17:03

Merge branch 'develop' of github.com:palantir/blueprint into gg/query…

d8f4d32

…list-modifiers

IFilm interface, better names, filmSelectProps

ff3038f

modifiers.filtered => modifiers.matchesPredicate

eb192d1

giladgray mentioned this pull request Jan 25, 2018

Allow item renderers to return null #2035

Closed

3 tasks

adidahiya approved these changes Jan 25, 2018

View reviewed changes

giladgray merged commit 59830d5 into develop Jan 25, 2018

giladgray deleted the gg/querylist-modifiers branch January 25, 2018 19:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QueryList itemRenderer prop & renderItem method #1877

QueryList itemRenderer prop & renderItem method #1877

giladgray commented Dec 9, 2017 •

edited

Loading

blueprint-bot commented Dec 9, 2017

blueprint-bot commented Dec 15, 2017

adidahiya left a comment

adidahiya Dec 15, 2017

adidahiya Dec 15, 2017

adidahiya Dec 15, 2017

adidahiya Dec 15, 2017

giladgray Dec 15, 2017 •

edited

Loading

adidahiya Dec 15, 2017

hellochar left a comment

hellochar Dec 15, 2017

giladgray Dec 15, 2017

hellochar Dec 15, 2017

giladgray Dec 15, 2017

themadcreator left a comment

themadcreator Dec 15, 2017

themadcreator Dec 15, 2017

themadcreator Dec 15, 2017

themadcreator Dec 15, 2017

giladgray Jan 25, 2018

themadcreator Dec 15, 2017

giladgray Jan 25, 2018

blueprint-bot commented Jan 25, 2018

giladgray commented Jan 25, 2018

adidahiya Jan 25, 2018

giladgray Jan 25, 2018

		@@ -107,6 +108,13 @@ export interface IQueryListRendererProps<T> extends IProps {
		*/
		handleKeyUp: React.KeyboardEventHandler<HTMLElement>;

		@@ -11,18 +11,16 @@ import * as React from "react";
		import * as sinon from "sinon";

		// this is an awkward import across the monorepo, but we'd rather not introduce a cyclical dependency or create another package

QueryList itemRenderer prop & renderItem method #1877

QueryList itemRenderer prop & renderItem method #1877

Conversation

giladgray commented Dec 9, 2017 • edited Loading

Notable Changes

blueprint-bot commented Dec 9, 2017

fix the tests

blueprint-bot commented Dec 15, 2017

Merge branch 'master' of github.com:palantir/blueprint into gg/querylist-modifiers

adidahiya 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

giladgray Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellochar 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

themadcreator 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Jan 25, 2018

modifiers.filtered => modifiers.matchesPredicate

giladgray commented Jan 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giladgray commented Dec 9, 2017 •

edited

Loading

giladgray Dec 15, 2017 •

edited

Loading