Skip to content
This repository has been archived by the owner on Dec 30, 2022. It is now read-only.

feat(Conditional): add connectStateResults connector #357

Merged
merged 7 commits into from
Sep 25, 2017

Conversation

mthuret
Copy link
Contributor

@mthuret mthuret commented Sep 20, 2017

Add a new connector that will remove the need of using the createConnector API to perform some conditional rendering. Could be used for any needs requiring access to the searchState or the searchResults .

See: #233

  • Add a warning for people using createConnector

@algobot
Copy link
Contributor

algobot commented Sep 20, 2017

Deploy preview ready!

Built with commit 8568c1b

https://deploy-preview-357--react-instantsearch.netlify.com

@algobot
Copy link
Contributor

algobot commented Sep 20, 2017

Deploy preview ready!

Built with commit 16c18e8

https://deploy-preview-357--react-instantsearch.netlify.com

@mthuret mthuret requested review from vvo and Haroenv September 20, 2017 12:56
@Haroenv
Copy link
Contributor

Haroenv commented Sep 20, 2017

Loving this change @mthuret, how would it look like with multi-query, is that with searchState?

return <div>{content}</div>;
});
const Content = connectStateResults(
({ searchState }) =>
Copy link
Contributor

@Haroenv Haroenv Sep 20, 2017

Choose a reason for hiding this comment

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

I'm wondering if we can't call this: ({ state, results })

Copy link
Contributor Author

@mthuret mthuret Sep 20, 2017

Choose a reason for hiding this comment

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

It's called searchState and searchResults everywhere especially in the documentation when looking for the structure so I'd rather keep the same name. If a user wants to call them state or results, they can definitely do it.

@mthuret
Copy link
Contributor Author

mthuret commented Sep 20, 2017

@Haroenv, I'm not sure to understand your question regarde multi query? (multi indices?)
The searchState contains all the data, you can see in the doc how it's formatted in that case.

* import { InstantSearch, Hits } from 'react-instantsearch/dom';
* import { connectStateResults } from 'react-instantsearch/connectors';
*
* const Content = connectStateResults(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having a little doubts about the name here. I think we can find something that describes intent rather than what it is.

connectConditional describes the use case better IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be use in the future for future use case such as retrieving the userInfo field you can get with query rules (and probably a usage with analytics also). Then we need a name that could represent this. Cf discussion in the issue thread. If you have a better name you can still propose it.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no API feature in this connector that has any conditional mean (no if/else) so naming it conditional would be not the best to me too.

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.

Minor comments, LGTM!

The query {searchState.query} exists
</div>
: <div>No query</div>
);
```

## Displaying content when there's no results
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have an example where we use data from multiple indices? I think we said something like "searchResults, allSearchResults".?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I can add the code of the story for multi indices here 👍

@@ -19,8 +19,9 @@ import { getResults } from '../core/indexUtils';
* @example
* import React from 'react';
*
* import { connectHits, Highlight, InstantSearch } from 'react-instantsearch/dom';
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this a mistake previously?

Copy link
Contributor

Choose a reason for hiding this comment

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

connectHits is from /connectors, so yes

});
const Content = connectStateResults(
({ searchState, searchResults }) =>
searchResults && searchResults.nbHits !== 0 ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

N.I.C.E

Copy link
Contributor

Choose a reason for hiding this comment

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

looks even nicer in future JS:

searchResults?.nbHits !== 0 ? 

@mthuret
Copy link
Contributor Author

mthuret commented Sep 22, 2017

warning added cc @vvo @Haroenv

onlyGetProvidedPropsUsage &&
!connectorDesc.displayName.startsWith('Algolia')
) {
// eslint-disable-next-line no-console
Copy link
Contributor

@Haroenv Haroenv Sep 22, 2017

Choose a reason for hiding this comment

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

I LOVE this change!

@mthuret mthuret merged commit 462df5f into master Sep 25, 2017
@mthuret mthuret deleted the feat/connect-conditional branch September 25, 2017 15:01
mthuret pushed a commit that referenced this pull request Sep 26, 2017
<a name="4.1.2"></a>
## [4.1.2](v4.1.1...v4.1.2) (2017-09-26)

### Features

* **Conditional:** add connectStateResults connector ([#357](#357)) ([462df5f](462df5f))
mthuret pushed a commit that referenced this pull request Oct 9, 2017
* New design

* Improve landing

* Add favicon

* New hero + proper meta

* Fix meta

* Clea css and fix some colors

* Reimplement demos

* Improve deoc + mobile menu

* New links for examples

* style(highlighting): adapt stylesheet

* Clean CSS

* Update front

* Edit code highlight css

* fix(sidebar): do not apply when not enough content

* chore(doc): fix doc sidebar (tested this time)

* New doc ready

* Update readme-logo.png

* wip

* Fix error for netlify

* Update .btn-cta style

* Fix issues

* Update communityHeader.json

* Fix hero links

* Update yarn.lock

* Fix UI

* Fix linter

* Fix linter JS

* chore(deps): update dependency eslint to v4.7.2 (#360)

* chore(deps): update dependency expo to ^21.0.0 (#358)

* chore(deps): update dependency expo to v^21.0.0

* fix tests

* chore(deps): update dependency jest-expo to v21.0.2 (#359)

* chore(argos): deactive argos

* docs(examples): add source code links

* feat(Conditional): add connectStateResults connector (#357)

* feat(Conditional): add connectStateResults connector

* feat(Conditiona): add warning and examples

* docs(tourism): fix capacity selector (#373)

* docs(RefinementList): fix defaultRefinement propTypes (#374)

* chore(deps): update dependency @storybook/addon-actions to v3.2.10 (#361)

* chore(deps): update dependency @storybook/addon-actions to v3.2.10

* chore: fix PR and group storybook updates

* chore(deps): update dependency eslint-config-prettier to v2.6.0 (#367)

* chore(deps): update dependency eslint-plugin-react to v7.4.0 (#369)

* chore(deps): update dependency postcss to v6.0.12 (#372)

* chore(deps): update dependency react to v15.6.2 (#379)

* chore(deps): update dependency react-test-renderer to v15.6.2 (#381)

* docs(Toggle): add requirements to cover null values use case (#375)

* chore(deps): update dependency uglify-js to v3.1.2 (#368)

* chore(deps): update dependency prop-types to v15.6.0 (#377)

* chore(deps): update dependency react-addons-test-utils to v15.6.2 (#378)

* chore(deps): update dependency react-dom to v15.6.2 (#380)

* chore(node): update version

* chore(release): ensure yarn boot installs devDeps (#385)

Since we switched to pinned devDeps, we need to ensure they are
installed, which is not the case when yarn boot is ran with
NODE_ENV=production (releases).

* v4.1.2

<a name="4.1.2"></a>
## [4.1.2](v4.1.1...v4.1.2) (2017-09-26)

### Features

* **Conditional:** add connectStateResults connector ([#357](#357)) ([462df5f](462df5f))

* chore(deps): update examples to react-instantsearch v4.1.2

* chore(release): fix release script

* chore(deps): update dependency babel-eslint to v8.0.1 (#389)

* chore(deps): update dependency eslint-plugin-jest to v21.2.0 (#391)

* chore(deps): update dependency jest to v21.2.0 (#392)

* chore(deps): renovate @storybook packages packages (#394)

* chore(deps): update dependency react-scripts to v1.0.14 (#395)

* chore(deps): update dependency compression to v1.7.1 (#396)

* chore(deps): update dependency jest-cli to v21.2.0 (#393)

* chore(deps): update dependency babel-jest to v21.2.0 (#390)

* chore(security): use node 8.6.0

node 8.5.0 has a sec vul: https://nodejs.org/en/blog/vulnerability/september-2017-path-validation/

* Fixing broken link (#402)

* docs(highlight): prettier and fix import (#413)

The import was from `InstantSearch`, while it should've been from `react-instantsearch/dom`

* tests(enzyme): upgrade to v3 (#397)

* tests(enzyme): upgrade to v3

* fix yarn lock

* chore(deps): update dependency jest to v21.2.1 (#401)

* docs(createConnector): add links for more info to deprecation (#403)

It's always useful to immediatly learn more and learn the solution of some problem that has been warned to you if available. React itself has short links for warnings (fb.me, we could also do that for alg.li, but maybe that's for later)

* chore(deps): renovate @storybook packages packages (#416)

* chore(deps): update dependency react-native-scripts to v1.5.0 (#399)

* chore(deps): update dependency extract-text-webpack-plugin to v3.0.1 (#418)

* chore(deps): update dependency enzyme to v3.1.0 (#421)

* chore(deps): update dependency conventional-changelog-cli to v1.3.4 (#408)

* chore(deps): update dependency eslint to v4.8.0 (#407)

* chore(deps): update dependency postcss to v6.0.13 (#422)

* chore(deps): update dependency enzyme-adapter-react-15 to v1.0.1 (#420)

* chore(deps): update dependency prettier to v1.7.4 (#398)

* chore(deps): update dependency material-ui to v0.19.3 (#411)

* chore(deps): update dependency uglify-js to v3.1.3 (#410)

* chore(deps): update dependency jest-cli to v21.2.1 (#400)

* chore(deps): update dependency eslint-plugin-jasmine to v2.8.6 (#409)

* chore(deps): update dependency style-loader to v0.19.0 (#419)

* update yarn.lock

* New design

* Improve landing

* Add favicon

* New hero + proper meta

* Fix meta

* Clea css and fix some colors

* Reimplement demos

* Improve deoc + mobile menu

* New links for examples

* style(highlighting): adapt stylesheet

* Clean CSS

* Update front

* Edit code highlight css

* fix(sidebar): do not apply when not enough content

* chore(doc): fix doc sidebar (tested this time)

* New doc ready

* Update readme-logo.png

* wip

* Fix error for netlify

* Update .btn-cta style

* Fix issues

* Update communityHeader.json

* Fix hero links

* Update yarn.lock

* Fix UI

* Fix linter

* Fix linter JS

* docs(examples): add source code links

* update yarn.lock

* update yarn.lock

* update yarn.lock
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants