Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(instantsearch-manager): provide a fn for setting SearchParameters #1471

Merged
merged 3 commits into from
Oct 26, 2016

Conversation

bobylito
Copy link
Contributor

FIX #1342

I haven't been able to do the tests yet :/ Working on it.

@algobot
Copy link
Contributor

algobot commented Oct 25, 2016

By analyzing the blame information on this pull request, we identified @Morhaus, @vvo and @mthuret to be potential reviewers

@@ -4,6 +4,19 @@ import {omit, has} from 'lodash';
import {shallowEqual, getDisplayName} from './utils';

/**
* @typedef {object} ConnectorDescription
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am unsure on the context of this addition, can you detail it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started exploring the possibility of writing the widget, and didn't have a proper documentation for the object that could be passed to createConnector. This is its documentation, and I thought maybe we could keep it since it was already done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure, if it currently does not have any output in our documentation website I would say we do not need it for now.

@@ -57,6 +57,9 @@ function validateNextProps(props, nextProps) {
* @propType {string} appId - The Algolia application id.
* @propType {string} apiKey - Your Algolia Search-Only API key.
* @propType {string} indexName - The index in which to search.
* @propType {function=identity} configureSearchParameters - function to tweak the parameters sent to Algolia.
* It is executed after the SearchParameters are resolved from the widgets. It can contains some
* logic to conditionally apply some parameters based on the state. Signature `SearchParameters => SearchParameters`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need to provide a link to the helper documentation on the SearchParameter object and what can be done.

@vvo
Copy link
Contributor

vvo commented Oct 25, 2016

Should we also write a guide section on configuring Algolia search parameters like distinct? #1315

@@ -57,6 +57,9 @@ function validateNextProps(props, nextProps) {
* @propType {string} appId - The Algolia application id.
* @propType {string} apiKey - Your Algolia Search-Only API key.
* @propType {string} indexName - The index in which to search.
* @propType {function=identity} configureSearchParameters - function to tweak the parameters sent to Algolia.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What we want to convey is that by default there's nothing being done. By saying the default value is "identity", any user not knowing what identity functions are (like I was at some point) may be confused.

Should we make sure everyone understands what "identity" means here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a pretty good question, I have no idea if this concept is well known or if we should make it more approachable. The nice thing of using identity is that it convey perfectly what it means, there is no approximation about its meaning, on the contrary of using words. On the other hand, if you don't know this vocab, you don't know it...

What do you think @mthuret? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some information about the behaviour of identity. Hope it's enough :D

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely we should explain what identity means because I'm sure some people will not know, and that can be frustrating. Your explanation is fine, and associated with an example (on the guide you mention @vvo ?), it would be imho nice :)

Copy link
Contributor

@vvo vvo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -57,6 +57,10 @@ function validateNextProps(props, nextProps) {
* @propType {string} appId - The Algolia application id.
* @propType {string} apiKey - Your Algolia Search-Only API key.
* @propType {string} indexName - The index in which to search.
* @propType {object} [searchParameters] - Object containing query parameters to be sent to Algolia.
* It will be overriden by the search parameters resolved via the widgets. Typical use case:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the "Typical use case"

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.

None yet

4 participants