Skip to content

Commit

Permalink
fix(numericRefinementList): Correctly apply active class
Browse files Browse the repository at this point in the history
Fixes #1010

The `shouldComponentUpdate` of `RefinementList` was checking if the
`facetValues` from the props where actually changed before issuing
a new rendering. It turns out that in `numericRefinementList`, the
new `facetValues` was always equal to the already set one. More
precisely, the filters seemed to already be set in the props before
reaching `shouldComponentUpdate`.

Turns out that we were editing a variable in place instead of working
on a copy so the internal props where changing without the component
really noticing.

I updated the `render` method of the `numericRefinementList` to pass
a new array instead of working on the same copy over and over again.

I refactored the `shouldComponentUpdate` and `_toggleRefinement`
methods by splitting in several variables in my quest to find the
issue. Thought I might keep it that way.

I added an explicit regression test to make sure we are really not
touching existing variables.

Now the current and next props are really different, and the full
render is propagated and the correct classes applied.# Possible types : feat, fix, refactor, chore, style, perf, test, docs
  • Loading branch information
pixelastic committed May 17, 2016
1 parent 6b8e34e commit 7cca9a4
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 8 deletions.
5 changes: 4 additions & 1 deletion src/components/RefinementList/RefinementList.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@ class RefinementList extends React.Component {
}

shouldComponentUpdate(nextProps, nextState) {
return nextState !== this.state || !isEqual(this.props.facetValues, nextProps.facetValues);
let isStateDifferent = nextState !== this.state;
let isFacetValuesDifferent = !isEqual(this.props.facetValues, nextProps.facetValues);
let shouldUpdate = isStateDifferent || isFacetValuesDifferent;
return shouldUpdate;
}

refine(facetValueToRefine, isRefined) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import React from 'react';
import expect from 'expect';
import sinon from 'sinon';
import jsdom from 'jsdom-global';
import cloneDeep from 'lodash/lang/cloneDeep';

import expectJSX from 'expect-jsx';
import numericRefinementList from '../numeric-refinement-list.js';
Expand Down Expand Up @@ -183,6 +184,28 @@ describe('numericRefinementList()', () => {
expect(helper.search.calledOnce).toBe(true, 'search called once');
});

it('does not alter the initial options when rendering', () => {
// Note: https://github.com/algolia/instantsearch.js/issues/1010
// Make sure we work on a copy of the initial facetValues when rendering,
// not directly editing it

// Given
let initialOptions = [{start: 0, end: 5, name: '1-5'}];
let initialOptionsClone = cloneDeep(initialOptions);
let testWidget = numericRefinementList({
container,
attributeName: 'price',
options: initialOptions
});

// When
testWidget.render({state, results, createURL});

// Then
expect(initialOptions).toEqual(initialOptionsClone);
});


afterEach(() => {
numericRefinementList.__ResetDependency__('ReactDOM');
numericRefinementList.__ResetDependency__('autoHideContainerHOC');
Expand Down
17 changes: 10 additions & 7 deletions src/widgets/numeric-refinement-list/numeric-refinement-list.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,15 +93,18 @@ function numericRefinementList({
templates
});

this._toggleRefinement = facetValue => helper
.setState(refine(helper.state, attributeName, options, facetValue))
.search();
this._toggleRefinement = (facetValue) => {
let refinedState = refine(helper.state, attributeName, options, facetValue);
helper.setState(refinedState).search();
};
},
render: function({results, state, createURL}) {
let facetValues = options.map(facetValue => {
facetValue.isRefined = isRefined(state, attributeName, facetValue);
facetValue.attributeName = attributeName;
return facetValue;
let facetValues = options.map((facetValue) => {
return {
...facetValue,
isRefined: isRefined(state, attributeName, facetValue),
attributeName: attributeName
};
});

// Bind createURL to this specific attribute
Expand Down

0 comments on commit 7cca9a4

Please sign in to comment.