[Labs] Suggest component #1499

llorca · 2017-08-24T21:41:55Z

Checklist

Include tests
Update documentation

Changes proposed in this pull request:

Suggest component composes Popover, InputGroup and QueryList

Screenshot

blueprint-bot · 2017-08-24T21:49:09Z

Update docs

Preview: documentation
Coverage: core | datetime

giladgray · 2017-08-24T21:45:04Z

packages/labs/src/components/select/suggest.tsx

+        const { handleKeyDown, handleKeyUp } = listProps;
+        const inputValue: string = isTyping
+            ? query : selectedItem
+                ? inputValueRenderer(selectedItem) : "";


alignment to clarify double-ternary

const inputValue: string = isTyping ? query : (selectedItem ? inputValueRenderer(selectedItem) : "");

giladgray · 2017-08-24T21:46:03Z

packages/labs/src/components/select/suggest.tsx

+                        onChange={this.handleQueryChange}
+                    />
+                </div>
+                <div onKeyDown={handleKeyDown} onKeyUp={handleKeyUp}>


why can't these handlers go on Menu? shouldn't need an extra wrapper element.

We can have Menu support ul HTML props in a separate PR (and then refactor all the labs components to use that)

giladgray · 2017-08-24T21:46:29Z

packages/labs/src/components/select/suggest.tsx

+                <div
+                    onKeyDown={this.getTargetKeyDownHandler(handleKeyDown)}
+                    onKeyUp={this.state.isOpen ? handleKeyUp : undefined}
+                    onClick={this.handleInputClick}


can these handlers go on InputGroup? you'll need to invoke corresponding inputProps.on* in each handler.

I'll try and report back

giladgray · 2017-08-24T21:48:04Z

packages/labs/src/components/select/suggest.tsx

+            this.input.blur();
+            this.setState({ isOpen: false });
+        } else {
+            this.setState({ isOpen: closeOnSelect ? nextOpenState : true });


prefer else if (closeOnSelect) { setState( true ) } instead of the no-op nextOpenState in ternary

✅ wait, i think i get it and this code is actually ok. nextOpenState is param so it needs to make it into state somehow, which my suggestion does not allow.

I'm not sure what you mean by no-op, but your suggestion is not equivalent to what I have.

This logic is messed up anyway, I'm gonna handle closeOnSelect stuff where it belongs: handleItemSelect

giladgray · 2017-08-24T21:48:16Z

packages/labs/src/components/select/suggest.tsx

+
+        if (this.input !== document.activeElement) {
+            // the input is no longer focused so we can close the popover
+            this.input.blur();


why blur if we already know it's not focused?

Good question, it's because handleItemSelect always refocuses the input. Now it only does with closeOnSelect={false} ✅

giladgray · 2017-08-24T21:49:18Z

packages/labs/src/components/select/suggest.tsx

+    private handlePopoverWillClose = () => {
+        const { popoverProps = {} } = this.props;
+
+        // resetting the query when the popover close


no resetOnClose?

We always reset on close here. If we don't, you can end up with the filtered list desynced from what's shown in the input (because we don't show query in the input at all times)

gotcha, cool. please update comment to clarify why it always resets.

dmackerman · 2017-08-24T22:24:03Z

Neat. Will this support InputGroup props so that icons can be applied and such?

llorca · 2017-08-24T22:25:13Z

@dmackerman you bet

blueprint-bot · 2017-08-24T22:50:28Z

Fix errors

Preview: documentation
Coverage: core | datetime

llorca · 2017-08-24T23:34:26Z

Not sure why this commit broke the build... The change is literally one word 🤔

- `Suggest` behaves similarly to `Select`, except it renders a text input as Popover target instead of arbitrary children.
+ `Suggest` behaves similarly to `Select`, except it renders a text input as the `Popover` target instead of arbitrary children.

giladgray · 2017-08-25T02:34:34Z

yeah not sure about the build... looks suspiciously like a test flake cuz it's about focus. 😕

llorca · 2017-08-25T17:58:09Z

Flaking test:

  <Overlay>
    Focus management
      ✖ returns focus to overlay if enforceFocus=true
        Chrome 60.0.3112 (Mac OS X 10.12.6)
      Error: Uncaught AssertionError: expected <button></button> to not equal <button></button> (test/index.ts:4311)

@giladgray how can I fix that? The underlying Popover has enforceFocus={false} and this test is about enforceFocus={true}:
https://github.com/palantir/blueprint/blob/al/suggest2/packages/labs/src/components/select/suggest.tsx#L138

llorca · 2017-08-25T18:04:22Z

Well that test actually fails on master too...

blueprint-bot · 2017-08-25T21:38:26Z

Update docs

Preview: documentation
Coverage: core | datetime

cmslewis · 2017-08-25T22:45:23Z

@llorca the "Open popover on key down" option doesn't appear to work:

cmslewis

Bug: openOnKeyDown doesn't appear to work.

blueprint-bot · 2017-08-26T17:02:59Z

Set proper state and fix keyDown handler

Preview: documentation
Coverage: core | datetime

cmslewis

Looks good overall! Left some questions for future reference.

cmslewis · 2017-08-27T00:43:00Z

packages/labs/src/components/select/suggest.tsx


        // reset the query when the popover close, make sure that the list
        // isn't filtered on when the popover opens next
-        this.setState({ query: "" });
+        this.setState({
+            activeItem: selectedItem ? selectedItem : this.props.items[0],


nit: Prefer explicit selectedItem != null. I like to avoid loose falsy checks.

cmslewis · 2017-08-27T00:44:22Z

packages/labs/src/components/select/suggest.tsx

+            ...restProps,
+        } = this.props;
+
+        return <this.TypedQueryList


nit: typical format for us is:

return ( // indented component );

cmslewis · 2017-08-27T00:45:28Z

packages/labs/src/components/select/suggest.tsx

+        const { isTyping, selectedItem, query } = this.state;
+        const { ref, ...htmlInputProps } = inputProps;
+        const { handleKeyDown, handleKeyUp } = listProps;
+        const inputValue: string = isTyping


Can we do away with the explicit : string type here?

cmslewis · 2017-08-27T00:47:42Z

packages/labs/src/components/select/suggest.tsx

+    private handleInputFocus = (event: React.SyntheticEvent<HTMLInputElement>) => {
+        const { openOnKeyDown, inputProps = {} } = this.props;
+
+        this.selectText();


Worth adding a selectAllOnFocus: boolean prop like we do for NumericInput?

cmslewis · 2017-08-27T00:54:36Z

packages/labs/src/components/select/suggest.tsx

+
+        if (!closeOnSelect) {
+            this.input.focus();
+            this.selectText();


Is this line necessary even though focus'ing will trigger handleInputFocus and subsequently invoke this.selectText()? (Per the requestAnimationFrame comment above, I'm assuming focus doesn't happen synchronously here?)

All comments appear to be addressed

cmslewis · 2017-08-29T17:35:40Z

@giladgray etc. I'm gonna merge this and then do an audit to make sure controlled mode works as expected before we release. Might even...

(••)
( ••)>⌐■-■
(⌐■_■) ...write some tests.

EDIT: Eh, actually, I'll just do that here before merging. There are also a couple other things I want to fix that I noticed in a second pass.

blueprint-bot · 2017-08-29T22:56:43Z

Fix param name

Preview: documentation | table
Coverage: core | datetime

cmslewis · 2017-08-29T22:59:42Z

Brought test coverage up to 90%.

sushain97 · 2017-08-30T21:34:23Z

If I understand this component correctly, it fulfills #1407 (please feel free to close the issue if I'm right).
@mfedderly, probably worth switching to this to easily get rid of the double input box problem.

Antoine Llorca added 2 commits August 24, 2017 14:37

Initial Suggest implementation

7f0e1f2

Update docs

00169b7

llorca requested a review from giladgray August 24, 2017 21:41

llorca mentioned this pull request Aug 24, 2017

Initial Suggest implementation #1497

Closed

3 tasks

giladgray previously requested changes Aug 24, 2017

View reviewed changes

Clarify double ternary

7b6fddf

Antoine Llorca added 3 commits August 24, 2017 15:28

closeOnSelect is true by default

ed46d34

Address comments

e99c7d5

Add openOnKeyDown prop

d9cf645

llorca requested a review from giladgray August 24, 2017 22:30

Fix errors

20054a3

Update docs

cfcd57a

cmslewis suggested changes Aug 25, 2017

View reviewed changes

Set proper state and fix keyDown handler

0f4dc5a

llorca requested a review from cmslewis August 26, 2017 17:05

cmslewis approved these changes Aug 27, 2017

View reviewed changes

cmslewis added P1 Package: labs labels Aug 27, 2017

cmslewis added 4 commits August 29, 2017 11:26

Write test stubs for Suggest

0affb8f

Add tests

9180678

Fix typo in selectTests.tsx

e614ac1

Move listeners to <InputGroup>, write more tests

d6650d0

cmslewis changed the title ~~[Labs] New Suggest component~~ [Labs] Suggest component Aug 29, 2017

Fix param name

18cbbfc

cmslewis merged commit 8398411 into master Aug 29, 2017

cmslewis deleted the al/suggest2 branch August 29, 2017 23:00

cmslewis mentioned this pull request Aug 30, 2017

Prepare Release 1.27.0 #1513

Merged

[Labs] Suggest component #1499

[Labs] Suggest component #1499

Conversation

llorca commented Aug 24, 2017 • edited by cmslewis Loading

Checklist

Changes proposed in this pull request:

Screenshot

blueprint-bot commented Aug 24, 2017

Update docs

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

dmackerman commented Aug 24, 2017

llorca commented Aug 24, 2017

blueprint-bot commented Aug 24, 2017

Fix errors

llorca commented Aug 24, 2017

giladgray commented Aug 25, 2017

llorca commented Aug 25, 2017

llorca commented Aug 25, 2017

blueprint-bot commented Aug 25, 2017

Update docs

cmslewis commented Aug 25, 2017

cmslewis left a comment

Choose a reason for hiding this comment

blueprint-bot commented Aug 26, 2017

Set proper state and fix keyDown handler

cmslewis 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

cmslewis commented Aug 29, 2017 • edited Loading

blueprint-bot commented Aug 29, 2017

Fix param name

cmslewis commented Aug 29, 2017

sushain97 commented Aug 30, 2017

llorca commented Aug 24, 2017 •

edited by cmslewis

Loading

cmslewis commented Aug 29, 2017 •

edited

Loading