Adding resetOnSelect behavior to created item

[alexisvisco]

Fixes #4103

After investigating the source code I found an issue with the resetOnSelect when there is an item creation. The current behavior when an item is created is by default reset the query event if resetOnSelect is set to false

Changes proposed in this pull request:

This PR change the behavior when an item is created and if resetOnSelect is true.
If resetOnSelect is false the query is not reset.

---

The code change is minimal so the review should be fast :)

[palantirtech]

Thanks for your interest in palantir/blueprint, @alexisvisco! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

[adidahiya]

This seems like a reasonable change, would you mind adding a unit test @alexisvisco?

FYSA @cmslewis

[cmslewis]

@alexisvisco Are you seeing this bug in any of the higher-order components that use QueryList (i.e., Select, Suggest, MultiSelect, Omnibar) or just when using QueryList directly? Code change looks reasonable, but would be helpful to see a repro if you have one.

[alexisvisco]

This bug is in all components that use QueryList that allow to change resetOnSelect and allow creating new item. Again this bug is only when you are creating new item, you create it, but in a UX perspective you are selecting it because there is no differences (a part from the fact that you are creating an item which is transparent for the user).

@cmslewis instead of a repo I have made a video that demonstrate the change between the actual production blueprint and my change https://youtu.be/C2m8WvYxpqw

Can you give me some hint to create tests please ? I don't known how to trigger a item selection / creation. I have looked your tests in the queryListTests.tsx but I don't really understand how it works.

Thanks you for your reviewing. :)

[cmslewis]

@alexisvisco

RE: Testing: You can add tests in the "query" describe block here. I'll let you commit them yourself so you can tinker, but I might try something like this:

it("resets the query after creating new item if resetOnSelect=true", () => {
    const onQueryChangeSpy = runResetOnSelectTest(true);
    assert.isTrue(onQueryChangeSpy.calledWith(""));
});

it("does not reset the query after creating new item if resetOnSelect=false", () => {
    const onQueryChangeSpy = runResetOnSelectTest(false);
    assert.isTrue(onQueryChangeSpy.notCalled);
});

function runResetOnSelectTest(resetOnSelect: boolean): sinon.SinonSpy {
    let triggerItemCreate: ((e: any) => void) | undefined;
    const onQueryChangeSpy = sinon.spy();
    // supply a custom renderer so we can hook into handleClick and invoke it ourselves later
    const createNewItemRenderer = sinon.spy(
        (_query: string, _active: boolean, handleClick: React.MouseEventHandler<HTMLElement>) => {
            triggerItemCreate = handleClick;
            return <div />;
        },
    );
    const queryList = shallow(
        <FilmQueryList
            {...testProps}
            // Must return something in order for item creation to work.
            createNewItemFromQuery={() => ({ title: "irrelevant", rank: 0, year: 0 })}
            createNewItemRenderer={createNewItemRenderer}
            onQueryChange={onQueryChangeSpy}
            resetOnSelect={resetOnSelect}
        />,
    );

    // Change the query to something non-empty so we can ensure it wasn't cleared.
    // Ignore this change in the spy.
    (queryList.instance() as QueryList<IFilm>).setQuery("some query");
    onQueryChangeSpy.resetHistory();

    assert.isDefined(triggerItemCreate, "query list should pass click handler to createNewItemRenderer");
    triggerItemCreate!({});

    return onQueryChangeSpy;
}

Let me know if that's not testing what you're trying to fix. To run these, you can cd into packages/select, slap an it.only on the tests you care about in order to run those only, and then run yarn test:karma:debug from your terminal to re-run tests every time you save.

[cmslewis]

nit: Since this appears on L419 as well, it might be nice to put this in a maybeResetQuery helper (after all the handle- function definitions, e.g. around L500).

private maybeResetQuery() {
  if (this.props.resetOnSelect) {
    this.setQuery("", true);
  }
}

That'd help avoid something like this happening again.

[adidahiya]

@alexisvisco sorry I didn't mention the new yarn format script in the dev docs -- adding that here #4118

[adidahiya]

friendly bump on this @alexisvisco

[alexisvisco]

I don't understand, the yarn format that I am running don't indicate any errors ... Can you help me ?

[adidahiya]

merged develop into your branch. here's the latest docs preview build