[select] feat(Suggest): sync activeItem with selectedItem on popover close

[skurger]

Make setActiveItem public so the parent components can reset the activeItem
when necessary.
When the Suggest Popover is closed and there is a selectedItem the activeItem
should always be set to selectedItem. When the Popover is opened again the
value of the component is reflected correctly.

Fixes #3769

Checklist

• Includes tests
• Update documentation

Changes proposed in this pull request:

Make setActiveItem in the QueryList component a public function so that parent components can call it. I saw similar behavior in setQuery already and I believe this is a decent compromise so that parent components (Select, Suggest, any other components that are added to this library in the future) do not have to add the boilerplate of keeping track of the active item since QueryList already does this. It is only in the case of closing a Popover that this "reset" needs to happen.

Reviewers should focus on:

The public / private function change. Also when trying to add a test to this I was having trouble getting the Popover onClosed function to fire. The code works in browser but not during test execution. Here is the code that I would like to add to suggestTests.tsx but I think that I'm missing some configuration that is allowing the child components to fire events. To me it looks like the code should work but I may be missing something obvious.

it("sets active item to the selected item when the popover is closed", () => {
    const ITEM_INDEX = 4;
    const wrapper = suggest();
    const queryList = ((wrapper.instance() as Suggest<IFilm>) as any).queryList; // private ref
    console.log("INITIAL STATE", queryList.state.activeItem, wrapper.state().selectedItem)
    simulateFocus(wrapper);
    assert.isTrue(wrapper.state().isOpen);
    selectItem(wrapper, ITEM_INDEX);
    assert.isFalse(wrapper.state().isOpen);
    simulateFocus(wrapper);
    assert.isTrue(wrapper.state().isOpen);
    simulateKeyDown(wrapper, Keys.ARROW_DOWN);
    console.log("AFTER KEY DOWN", queryList.state.activeItem, wrapper.state().selectedItem)
    console.log("BEFORE BLUR", queryList.state.activeItem, wrapper.state().selectedItem)
    simulateKeyDown(wrapper, Keys.ESCAPE);
    assert.isFalse(wrapper.state().isOpen);
    console.log("AFTER BLUR", queryList.state.activeItem, wrapper.state().selectedItem)
    assert.deepEqual(queryList.state.activeItem, wrapper.state().selectedItem);
});

Also if the test issue is resolved in this PR I'd suggest we also make this change for the Select component so that behavior is consistent throughout the library. I could either include those changes in this commit or open another PR after. Let me know the preferred way.

Screenshot

[adidahiya]

looks good, just some small comments

[adidahiya]

which part of your new test is failing? you might as well push it in this PR and we can continue to work on it. I agree that we should add this functionality to Select, it can be a separate PR.

[skurger]

Thanks for the feedback. I pushed the changes for the explicit checks. Let me look at the test again and clean it up a little bit. I'll push it up after that. I was trying to simulate the arrow key down when a popover was open so that the active item would change but not the selected item. When the popover was closed the active item should be reset back to the selected item.

[adidahiya]

great, thanks. for future reference, I'd prefer if you push new commits (including merges) to this branch instead of force-pushing. it makes reviewing incremental changes a little easier :)

[skurger]

Got it. I was trying to keep my fork in sync with all the new incoming changes. Will keep in mind for next time. I'll add the test tonight if I have time. The actual problem was that the active item wasn't changing on key down when the popover was open in the tests.

[skurger]

@adidahiya take a look at the test I just pushed up. If you look at the console output the active item is always the selectedItem. It seems that after simulating the arrow key down the active item doesn't actuallly change. I'm not sure if this is a limitation of the testing framework or if there is something obvious I'm missing here. I want to assert that an item is selected and the component has a value. The popover is then opened again and another item in the query list is set to active (like a user using the arrow keys to navigate the menu) and then the popover is closed without making a selection. When the popover is opened again the active item should equal the selectedItem.

[skurger]

Just bumping this thread so that we can try to button up this PR and get it merged. Let me know when you've had a chance to take a look.

[adidahiya]

@skurger I changed the behavior to be slightly simpler (and apparently more testable) by syncing the active item in componentDidUpdate. Since Suggest is controlling the Popover isOpen prop, this should be about the same as handling this in Popover onClosed. Can you confirm that your code comment ("...there isn't a flash of the activeItem on screen") is still true after my latest commits?

[skurger]

Looks good to me. Ran the code and tests and they look good. The only small thing which I'll leave up to you is that you can see the active item flash to the selected item when the popover is closing since the animation is still running after the active item is set to the selected item.

Good stuff, thanks for the help!

[adidahiya]

sorry for the delay here... I think I fixed the flash of content though. gonna go ahead and merge