Skip to content

Conversation

@wetneb
Copy link
Contributor

@wetneb wetneb commented Jun 9, 2019

What issue does this pull request resolve?
The description of the filterBy callback is not clear enough.

What changes did you make?
I added a description of the arguments and return value expected from this callback.

Is there anything that requires more attention while reviewing?
I don't think so.

@ericgio
Copy link
Owner

ericgio commented Jun 10, 2019

Hi @wetneb, thanks for the pull request. The docs have a dedicated section on filtering with a more in-depth description of the API. The props table is meant to be more of an overview of the available props.

@wetneb
Copy link
Contributor Author

wetneb commented Jun 11, 2019

I am not sure what would be the harm in adding that information there too? Would you prefer it to be a direct link to https://github.com/ericgio/react-bootstrap-typeahead/blob/master/docs/Filtering.md#filterby maybe?

Overall my point is it is a bit frustrating for the user to read about "a callback" without knowing what its signature is. I think it is reasonable to expect that a list of props also documents the types of each of these props.

@ericgio
Copy link
Owner

ericgio commented Jun 12, 2019

Yes, I think a direct link would help surface the in-depth docs. Adding the actual signature could be a more concise way of presenting the information directly (eg: (option: Object|String, props: Object) => boolean), but it doesn't look like that works very well within table markdown.

@ericgio
Copy link
Owner

ericgio commented Jun 12, 2019

This is great, thank you.

@ericgio ericgio merged commit 7025dfb into ericgio:master Jun 12, 2019
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.

2 participants