Always render suggestions – fix #133

[thibaudcolas]

Hey hey,

this is a PR to fix #133. It implements a new alwaysRenderSuggestions boolean prop, as we discussed. When alwaysRenderSuggestions={true}, the checks to decide whether to render the suggestions are bypassed. Also comes with some documentation on this new prop, and relevant tests.

I'd be keen to get a review of whether the tests are relevant and thorough enough, and make sure this new prop is used in the right places.

Also two things occurred to me while writing this PR:

• Having an example for this would only be relevant if it came with the kind of UI this is useful for. Do we want to make one? (another way to see this would be to have a list of "projects react-autosuggest is used on" somewhere).
• It might or might not be be obvious that in order for this to be useful, you need to have a list of suggestions to display even when the user hasn't typed anything yet (otherwise there's nothing to display, doh). Does this need to be stated in the docs?

[moroshko]

👍 for "elm", but why || 'elm' is required?

[thibaudcolas]

For those tests to be useful it is necessary to get suggestions even when there's no value in the input – that's why I use elm here. It's not what would happen in a real-world app but does the same job for the tests' purpose.

I added a comment to make this clearer.

[moroshko]

Overall looks great! Just a few minor comments above.

[thibaudcolas]

As those tweaks were quite minor I just rebased them into my previous commits:

• Fix spacing in variable declaration list
• toggleAlwaysRenderSuggestions -> setAlwaysRenderSuggestions
• Documentation changes
• Comment to make the test helper's intent clearer.

[moroshko]

Do we really need alwaysRenderSuggestions || here?
All tests pass without it, so I wonder if we can just remove it, or are we missing a test here?

[moroshko]

@ThibWeb Thanks for the tweaks!

I think I found an issue with the implementation.

I just added alwaysRenderSuggestions={true} to the CustomRenderer example (also had to change [] to people here).
Suggestions are initially rendered, as expected.
However, when I hit Down, the first suggestion isn't getting focus. Changing this line to:

if (!alwaysRenderSuggestions && isCollapsed) ...

solves the issue, but it doesn't feel right. isCollapsed is true even though suggestions are shown.

Basically, we have a prop and a state that control the view:

• alwaysRenderSuggestions - prop
• isCollapsed - state

I feel that the prop should set the initial state. Then, alwaysRenderSuggestions should be used to make sure that isCollapsed stays true no matter what. This way, isCollapsed will be true if and only if suggestions are shown.

What do you think?

[thibaudcolas]

Yes, I've had some trouble understanding how best to interact with isCollapsed – doesn't surprise me that I got it wrong. I'll revisit this and probably create a standalone example, as much as a help when developing as to demonstrate what the prop is for.

[bradbirnbaum]

Checking in on this pull - do we expect to take it soon?

[moroshko]

@ThibWeb Have you had a chance to revisit this PR?

[bradbirnbaum]

Checking in to see if we expect this to be released soon. Thanks

[thibaudcolas]

Hey there – sorry for the delay, I was on holidays for a few weeks.

I still intend to work on this, and will commit to get back with a better PR this weekend. Hopefully this will be releasable.

[jxodwyer]

@ThibWeb just wondering the status of this PR? I'd be happy to jump in and help out if you're running into issues or strapped for time.

[thibaudcolas]

Hey @jxodwyer, strapped for time indeed. If you want to pick this up by all means go for it! Two weeks ago I did some work on this and a relevant UI example. That makes it easier to see why this is useful, what is going on, and where it fails.

Useful links:

• Compiled UI example, for reference: https://rawgit.com/ThibWeb/react-autosuggest/demo/arr/demo/dist/index.html
• Branch I'm working from https://github.com/ThibWeb/react-autosuggest/tree/feature/always-render-suggestions

Next things to do IMHO:

• Look at moroshko's suggestions/hints above.
• Debug the current implementation with those hints and the examples as visual aids
• Make sure existing test suite still passes
• Add more tests for the cases that do not work here
• Update PR 🚀
• Figure out if this example needs a Codepen link (under moroshko's account like the others?)

I've added you as a contributor to https://github.com/ThibWeb/react-autosuggest, it's up to you if you prefer to use this as is or start from it in another repo 👌. Please let me know what you decide as I'll likely have more time next weekend.

[shprink]

This feature is much needed indeed! Thanks for your hard work guys, can't wait to see that merged!

[thibaudcolas]

@moroshko I have updated the PR, could you have a look?

I followed your suggestions, alwaysRenderSuggestions is now used to set the default value of isCollapsed, and to set shouldRenderSuggestions to always return true. Ideally I would have wanted to completely remove any references to alwaysRenderSuggestions in the Autosuggest component, but there are a few instances where isCollapsed and shouldRenderSuggestions are not the only criteria to determine what is rendered:

• https://github.com/ThibWeb/react-autosuggest/blob/master/src/Autosuggest.js#L85
• https://github.com/ThibWeb/react-autosuggest/blob/master/src/Autosuggest.js#L185

Those extra checks are useful, but having them within the Autosuggest component makes the code harder to change – there are too many places in the code at different levels of abstraction that are in charge of determining whether suggestions are shown or not. Ideally I think that a single isOpen/isCollapsed prop should be the one and only source of truth for whether suggestions are shown or not. A way to get towards this would be to change how isCollapsed is set in mapDispatchToProps, but that's outside of the scope of this PR.

Other than that, I added the relevant tests to check for:

• the unwanted behavior that the previous version of this PR produced.
• the wanted behavior when selecting a suggestion

And I updated https://rawgit.com/ThibWeb/react-autosuggest/demo/arr/demo/dist/index.html to make reviewing this easier.

The last thing that's missing would be a CodePen link – not too sure how to proceed here, since this isn't something we can collaborate on via PRs?

Edit: this PR is running in production at https://data1850.nz/chart 🚀

[rickhanlonii]

@moroshko just a heads up that merging this will cause some conflicts with #165. After this is merged I'll update #165 and add any tests necessary for both props to be active at once.

[thibaudcolas]

Just a heads up that if someone wants to use this sooner rather than later, I have a branch where this work is compiled and npm installable: https://github.com/ThibWeb/react-autosuggest/tree/feature/arr-compiled

I'd advise against relying on someone else's branch in a fork for something serious, but you can easily fork it to your own GitHub account to be more certain that it will stay how it is, and then npm install --save react-autossugest@git://github.com/ThibWeb/react-autosuggest.git#feature/arr-compiled with your own user / repo / branch.

[moroshko]

@ThibWeb I'm keen to review and merge this PR. Do you mind resolving the merge conflicts? (sorry, they appeared after I merged #165)

[carloscuatin]

@ThibWeb resolve conflicts please 😀

[shprink]

🙇 @ThibWeb I will pay you a beer when you come to France if you resolve the conflicts :)

[thibaudcolas]

Wuhu, conflicts merged! They were quite straightforward, only had to add the alwaysRenderSuggestions vars where they belong.

Regarding focusFirstSuggestion – when both this and alwaysRenderSuggestions are turned on, at the moment it only puts the autosuggest focus on the first suggestion when the user moves focus to the autosuggest field. Focusing one of the suggestions even though the autosuggest does not have focus feels strange to me (happy to be told I'm wrong).

The running example at https://rawgit.com/ThibWeb/react-autosuggest/demo/arr/demo/dist/index.html is up to date. @moroshko this might not pass the linting since your latest config updates – I tried to make it work but eslint-plugin-react@6.0.0-alpha.1 keeps complaining about unfound rule definitions.

[LuckUY]

Hi @moroshko,

Any update on this new feature? When do you think it will be merged?

Thanks

[carloscuatin]

nice @moroshko 🎉

[shprink]

Sending <3 to all of you!

[LuckUY]

@moroshko

I cannot see the new "alwaysRenderSuggestions" prop on Autosuggest.js (v 3.9.0)
Did you publish a new version?

Thanks

[moroshko]

alwaysRenderSuggestions is available in v4.0.0 now.