Skip to content

Conversation

@ET-CS
Copy link
Contributor

@ET-CS ET-CS commented Aug 13, 2020

This PR adds filterBy={() => true} to the AyncExample.

It's an issue when the server is filtering by multiple fields (such as name, family-name, email, tags, etc..)
and then, if the client labelKey is only by name (for example), no result will be shown.
Ones can set the labelKey to have all the fields inside or create filterBy array on all the fields (which is not always possible, and sometimes even the backend doesn't return all the fields to the client).

Also, when filtering server-side, it's a waste to filter again in client-side all results.

IMHO It's important thing to be in the example, as in our case, took us few hours to find this out and we unfortunately first tried to set the labelKey to ${name} ${email} and then override both renderInput, renderToken and renderMenuItemChildren to show only the name field...

Do not filter results in async example
@codecov
Copy link

codecov bot commented Aug 13, 2020

Codecov Report

Merging #578 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #578   +/-   ##
=======================================
  Coverage   99.84%   99.84%           
=======================================
  Files          44       44           
  Lines         658      658           
  Branches      133      133           
=======================================
  Hits          657      657           
  Misses          1        1           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca9cc68...2f88d99. Read the comment docs.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.848% when pulling 2f88d99 on ET-CS:master into ca9cc68 on ericgio:master.

@ericgio
Copy link
Owner

ericgio commented Aug 24, 2020

Hey @ET-CS, thanks for submitting this PR. Seems like a reasonable change. I've been debating whether this should actually be the default value in AsyncTypeahead, but I don't get too many comments one way or another, so I've left it as is for now. Sorry this issue tripped you up. Note that it is called out in the filterBy docs.

@ericgio ericgio merged commit 1ff2a27 into ericgio:master Aug 24, 2020
@ET-CS
Copy link
Contributor Author

ET-CS commented Sep 14, 2020

Thank you @ericgio . IMHO this should actually be the default value in AsyncTypeahead. Thanks for the great work on this project!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants