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

feat(connectToggleRefinement): implement canRefine & count #1588

Merged
merged 10 commits into from
Sep 20, 2018

Conversation

samouss
Copy link
Collaborator

@samouss samouss commented Sep 18, 2018

Summary

This PR adds canRefine & count on the provided props of the connectToggleRefinement connector. I didn't implement the count on the widget because it will be a breaking change only in the connector.

The count object has the following shape (see below). When the currentRefinement is false (unchecked) the user can pick the unchecked attribute. It displays the number of results that match the value provided to the Toggle widget. Once it's checked the count displays the total number of results (compute from the facet values).

type Count = {
  checked: number | null;
  unchecked: number | null;
};

Changes:

  • the computation of the SearchParameters (conjunctiveFacet -> disjunctiveFacet)
  • the computation of the providedProps (adds canRefine & count)
  • adds the noRefinement class on the Toggle root element

@samouss samouss requested a review from a team September 18, 2018 09:14
@samouss samouss force-pushed the feat/connect-toggle-count branch from db9ecc1 to 0f1c9d9 Compare September 18, 2018 09:15
@algobot
Copy link
Contributor

algobot commented Sep 18, 2018

Deploy preview for react-instantsearch ready!

Built with commit b279620

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

@algolia algolia deleted a comment from algobot Sep 18, 2018
@samouss samouss force-pushed the feat/connect-toggle-count branch from 23e226f to 5744cfd Compare September 18, 2018 09:53
expect(params.getDisjunctiveRefinements('facet')).toEqual(['val']);
});

it('does not refines the corresponding facet with `false`', () => {
Copy link
Member

Choose a reason for hiding this comment

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

refinesrefine

return { currentRefinement };

const facetValues = results && results.getFacetValues(attribute);
const facetValue =
Copy link
Member

Choose a reason for hiding this comment

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

These two variable names are confusing.

Copy link
Collaborator Author

@samouss samouss Sep 19, 2018

Choose a reason for hiding this comment

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

Any suggestions? I didn't find something else.

Copy link
Member

Choose a reason for hiding this comment

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

When I deal with both singular and plural variable names, I tend to go for allFacetValues and facetValue. WDYT?

Copy link
Collaborator Author

@samouss samouss Sep 19, 2018

Choose a reason for hiding this comment

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

To be honest I don't see a huge benefit, I'm used to the plural singular form like the original one. But I get your point.

Copy link
Member

Choose a reason for hiding this comment

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

When I came across these two lines I thought you were referring to the same variable. The benefit is just to avoid this confusion. This is subjective though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep I've renamed it.

@samouss samouss force-pushed the feat/connect-toggle-count branch from 5744cfd to 4494ea1 Compare September 19, 2018 18:05
@samouss samouss force-pushed the feat/connect-toggle-count branch from 4494ea1 to b279620 Compare September 20, 2018 08:41
@samouss samouss merged commit 40672dd into master Sep 20, 2018
@samouss samouss deleted the feat/connect-toggle-count branch September 20, 2018 09:06
samouss added a commit that referenced this pull request Sep 24, 2018
<a name="5.3.0"></a>
# [5.3.0](v5.2.3...v5.3.0) (2018-09-24)

### Bug Fixes

* **SSR:** bind getSearchParmaters to the component instance ([f34cb3d](f34cb3d))
* **GoogleMapsLoader:** pick google maps version ([#1540](#1540)) ([b14efcf](b14efcf))

### Features

* **connectToggleRefinement:** implement canRefine & count ([#1588](#1588)) ([40672dd](40672dd))
@VincentCtr
Copy link

VincentCtr commented Sep 25, 2018

@samouss it seems that this PR is causing a crash when we use ToggleRefinement and change the index used by InstantSearch widget

app.js:sourcemap:406316 Error: **FacetName** is not a retrieved facet.
    at SearchResults../node_modules/algoliasearch-helper/src/SearchResults/index.js.SearchResults.getFacetValues (app.js:sourcemap:55188)
    at Connector.getProvidedProps (app.js:sourcemap:281799)
    at Connector.getProvidedProps (app.js:sourcemap:282134)
    at new Connector (app.js:sourcemap:281975)
    at constructClassInstance (app.js:sourcemap:250944)
    at updateClassComponent (app.js:sourcemap:252666)
    at beginWork (app.js:sourcemap:253265)
    at performUnitOfWork (app.js:sourcemap:255591)
    at workLoop (app.js:sourcemap:255629)
    at renderRoot (app.js:sourcemap:255708)

I spent the whole day trying to figure out what happen and finally came back to 5.2.3

@samouss
Copy link
Collaborator Author

samouss commented Sep 25, 2018

Hi @VincentCtr, sorry about that. I may found out why it happens, the index you are switching on does have the facet that you provide to the Toggle? If you have an example of the issue it would be awesome, because I didn't manage to reproduce it. We provide a template to avoid the boilerplate. I've create a PR to solve the issue.

@VincentCtr
Copy link

Hi @samouss, thank you for this very quick fix !

@samouss
Copy link
Collaborator Author

samouss commented Sep 26, 2018

The PR has been merged and released (v5.3.1), let me know if you still have any issues.

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.

5 participants