fix(slider): Fix range slider pips and value 0 (#2350)

* fix(Slider): correctly disable pips * fix(connectRangeSlider): dont apply numeric refinement * test(range-slider): test pips & value 0 * fix(connectRangeSlider): correctly refine when no userSet min/max fix #2343
algolia · Sep 20, 2017 · fa0dc09 · fa0dc09
1 parent bc947ec
commit fa0dc09
Show file tree

Hide file tree

Showing 6 changed files with 255 additions and 30 deletions.
diff --git a/dev/app/init-builtin-widgets.js b/dev/app/init-builtin-widgets.js
@@ -487,27 +487,48 @@ export default () => {
       })
     );
 
-  storiesOf('RangeSlider').add(
-    'default',
-    wrapWithHits(container => {
-      window.search.addWidget(
-        instantsearch.widgets.rangeSlider({
-          container,
-          attributeName: 'price',
-          templates: {
-            header: 'Price',
-          },
-          max: 500,
-          step: 10,
-          tooltips: {
-            format(rawValue) {
-              return `$${Math.round(rawValue).toLocaleString()}`;
+  storiesOf('RangeSlider')
+    .add(
+      'default',
+      wrapWithHits(container => {
+        window.search.addWidget(
+          instantsearch.widgets.rangeSlider({
+            container,
+            attributeName: 'price',
+            templates: {
+              header: 'Price',
             },
-          },
-        })
-      );
-    })
-  );
+            max: 500,
+            step: 10,
+            tooltips: {
+              format(rawValue) {
+                return `$${Math.round(rawValue).toLocaleString()}`;
+              },
+            },
+          })
+        );
+      })
+    )
+    .add(
+      'without pips',
+      wrapWithHits(container => {
+        window.search.addWidget(
+          instantsearch.widgets.rangeSlider({
+            container,
+            attributeName: 'price',
+            min: 0,
+            max: 500,
+            pips: false,
+            step: 10,
+            tooltips: {
+              format(rawValue) {
+                return `$${Math.round(rawValue).toLocaleString()}`;
+              },
+            },
+          })
+        );
+      })
+    );
 
   storiesOf('HierarchicalMenu')
     .add(

diff --git a/src/components/Slider/Slider.js b/src/components/Slider/Slider.js
@@ -83,11 +83,9 @@ class Slider extends Component {
 
     return (
       <div {...props} className={className}>
-        {tooltips
-          ? <div className="ais-range-slider--tooltip">
-              {value}
-            </div>
-          : null}
+        {tooltips ? (
+          <div className="ais-range-slider--tooltip">{value}</div>
+        ) : null}
       </div>
     );
   };
@@ -101,9 +99,7 @@ class Slider extends Component {
 
     const snapPoints = this.computeSnapPoints({ min, max, step });
     const pitPoints =
-      pips === true || pips === undefined || pips === false
-        ? this.computeDefaultPitPoints({ min, max })
-        : pips;
+      pips === false ? [] : this.computeDefaultPitPoints({ min, max });
 
     return (
       <div className={this.isDisabled ? 'ais-range-slider--disabled' : ''}>

diff --git a/src/components/Slider/__tests__/Slider-test.js b/src/components/Slider/__tests__/Slider-test.js
@@ -21,4 +21,22 @@ describe('Slider', () => {
       .toJSON();
     expect(tree).toMatchSnapshot();
   });
+
+  it('should render without pips', () => {
+    const tree = renderer
+      .create(
+        <Slider
+          refine={() => undefined}
+          min={0}
+          max={500}
+          values={[0, 0]}
+          pips={false}
+          step={2}
+          tooltips={true}
+          shouldAutoHideContainer={false}
+        />
+      )
+      .toJSON();
+    expect(tree).toMatchSnapshot();
+  });
 });
diff --git a/src/components/Slider/__tests__/__snapshots__/Slider-test.js.snap b/src/components/Slider/__tests__/__snapshots__/Slider-test.js.snap
@@ -464,3 +464,100 @@ exports[`Slider should render correctly 1`] = `
   </div>
 </div>
 `;
+
+exports[`Slider should render without pips 1`] = `
+<div
+  style={
+    Object {
+      "display": "",
+    }
+  }
+>
+  <div
+    className="ais-root"
+  >
+    <div
+      className="ais-body"
+    >
+      <div
+        className=""
+      >
+        <div
+          className="rheostat rheostat-horizontal "
+          onClick={[Function]}
+          style={
+            Object {
+              "position": "relative",
+            }
+          }
+        >
+          <div
+            className="rheostat-background"
+          />
+          <div
+            aria-disabled={false}
+            aria-valuemax={500}
+            aria-valuemin={0}
+            aria-valuenow={0}
+            className="ais-range-slider--handle rheostat-handle ais-range-slider--handle-lower"
+            data-handle-key={0}
+            onClick={undefined}
+            onKeyDown={[Function]}
+            onMouseDown={[Function]}
+            onTouchStart={[Function]}
+            role="slider"
+            style={
+              Object {
+                "left": "0%",
+                "position": "absolute",
+              }
+            }
+            tabIndex={0}
+          >
+            <div
+              className="ais-range-slider--tooltip"
+            >
+              0
+            </div>
+          </div>
+          <div
+            aria-disabled={false}
+            aria-valuemax={500}
+            aria-valuemin={0}
+            aria-valuenow={0}
+            className="ais-range-slider--handle rheostat-handle ais-range-slider--handle-upper"
+            data-handle-key={1}
+            onClick={undefined}
+            onKeyDown={[Function]}
+            onMouseDown={[Function]}
+            onTouchStart={[Function]}
+            role="slider"
+            style={
+              Object {
+                "left": "0%",
+                "position": "absolute",
+              }
+            }
+            tabIndex={0}
+          >
+            <div
+              className="ais-range-slider--tooltip"
+            >
+              0
+            </div>
+          </div>
+          <div
+            className="rheostat-progress"
+            style={
+              Object {
+                "left": "0%",
+                "width": "0%",
+              }
+            }
+          />
+        </div>
+      </div>
+    </div>
+  </div>
+</div>
+`;
diff --git a/src/connectors/range-slider/__tests__/connectRangeSlider-test.js b/src/connectors/range-slider/__tests__/connectRangeSlider-test.js
@@ -184,4 +184,81 @@ describe('connectRangeSlider', () => {
       expect(helper.search.callCount).toBe(2);
     }
   });
+
+  it('should add numeric refinement when refining min boundary', () => {
+    const rendering = sinon.stub();
+    const makeWidget = connectRangeSlider(rendering);
+
+    const attributeName = 'price';
+    const widget = makeWidget({ attributeName, min: 0, max: 500 });
+
+    const helper = jsHelper(fakeClient, '', widget.getConfiguration());
+    helper.search = sinon.stub();
+
+    widget.init({
+      helper,
+      state: helper.state,
+      createURL: () => '#',
+      onHistoryChange: () => {},
+    });
+
+    {
+      // first rendering
+      expect(helper.getNumericRefinement('price', '>=')).toEqual([0]);
+      expect(helper.getNumericRefinement('price', '<=')).toEqual([500]);
+
+      const renderOptions = rendering.lastCall.args[0];
+      const { refine } = renderOptions;
+      refine([10, 30]);
+
+      expect(helper.getNumericRefinement('price', '>=')).toEqual([10]);
+      expect(helper.getNumericRefinement('price', '<=')).toEqual([30]);
+      expect(helper.search.callCount).toBe(1);
+
+      refine([0, undefined]);
+      expect(helper.getNumericRefinement('price', '>=')).toEqual([0]);
+      expect(helper.getNumericRefinement('price', '<=')).toEqual(undefined);
+    }
+  });
+
+  it('should refine on boundaries when no min/max defined', () => {
+    const rendering = sinon.stub();
+    const makeWidget = connectRangeSlider(rendering);
+
+    const attributeName = 'price';
+    const widget = makeWidget({ attributeName });
+
+    const helper = jsHelper(fakeClient, '', widget.getConfiguration());
+    helper.search = sinon.stub();
+
+    widget.init({
+      helper,
+      state: helper.state,
+      createURL: () => '#',
+      onHistoryChange: () => {},
+    });
+
+    {
+      expect(helper.getNumericRefinement('price', '>=')).toEqual(undefined);
+      expect(helper.getNumericRefinement('price', '<=')).toEqual(undefined);
+
+      const renderOptions = rendering.lastCall.args[0];
+      const { refine } = renderOptions;
+
+      refine([undefined, 100]);
+      expect(helper.getNumericRefinement('price', '>=')).toEqual(undefined);
+      expect(helper.getNumericRefinement('price', '<=')).toEqual([100]);
+      expect(helper.search.callCount).toBe(1);
+
+      refine([0, undefined]);
+      expect(helper.getNumericRefinement('price', '>=')).toEqual([0]);
+      expect(helper.getNumericRefinement('price', '<=')).toEqual(undefined);
+      expect(helper.search.callCount).toBe(2);
+
+      refine([0, 100]);
+      expect(helper.getNumericRefinement('price', '>=')).toEqual([0]);
+      expect(helper.getNumericRefinement('price', '<=')).toEqual([100]);
+      expect(helper.search.callCount).toBe(3);
+    }
+  });
 });
diff --git a/src/connectors/range-slider/connectRangeSlider.js b/src/connectors/range-slider/connectRangeSlider.js
@@ -132,14 +132,30 @@ export default function connectRangeSlider(renderFn) {
             currentValues[1] !== newValues[1]
           ) {
             helper.clearRefinements(attributeName);
-            if (!bounds.min || newValues[0] > bounds.min) {
+
+            const hasMin = bounds.min !== null && bounds.min !== undefined;
+            const minValueChanged =
+              newValues[0] !== null && newValues[0] !== undefined;
+
+            if (
+              (hasMin && minValueChanged && bounds.min < newValues[0]) ||
+              (!hasMin && minValueChanged)
+            ) {
               helper.addNumericRefinement(
                 attributeName,
                 '>=',
                 formatToNumber(newValues[0])
               );
             }
-            if (!bounds.max || newValues[1] < bounds.max) {
+
+            const hasMax = bounds.max !== null && bounds.max !== undefined;
+            const maxValueChanged =
+              newValues[1] !== null && newValues[1] !== undefined;
+
+            if (
+              (hasMax && maxValueChanged && bounds.max > newValues[1]) ||
+              (!hasMax && maxValueChanged)
+            ) {
               helper.addNumericRefinement(
                 attributeName,
                 '<=',