Removing pageSize from sliderProps (#2917)

LFDanLu · snowystinger · web-flow · commit 0f255bbe8aa8 · 2022-03-04T13:16:05.000-08:00
* removing pageSize from sliderProps

also forcing pageSize to be a multiple of step

* adding test for page size clamping

* using snap value to step for page size calc

* fixing logic to account for if step is less or greater than min/max

* fixing comment

* making math simpler for calculating page size

* exposing pageSize and step from useColorSliderState

memoizing step calc in useSliderState as well. Exposing pageSize and step from useColorSliderState allows us to remove extraneous useKeyboard from useColorSlider that was previously needed for pageUp/Down handling

Co-authored-by: Robert Snow &lt;rsnow@adobe.com&gt;
diff --git a/packages/@react-aria/slider/stories/Slider.stories.tsx b/packages/@react-aria/slider/stories/Slider.stories.tsx
@@ -28,10 +28,6 @@ storiesOf('Slider (hooks)', module)
     'single with aria label',
     () => <StorySlider aria-label="Size" onChange={action('onChange')} onChangeEnd={action('onChangeEnd')} showTip />
   )
-  .add(
-    'single with pageSize',
-    () => <StorySlider label="Degrees" onChange={action('onChange')} onChangeEnd={action('onChangeEnd')} minValue={0} maxValue={360} pageSize={15} formatOptions={{style: 'unit', unit: 'degree', unitDisplay: 'narrow'}} showTip />
-  )
   .add(
     'range',
     () => (<StoryRangeSlider
@@ -60,23 +56,6 @@ storiesOf('Slider (hooks)', module)
         unitDisplay: 'narrow'
       } as any} />)
   )
-  .add(
-    'range with pageSize',
-    () => (<StoryRangeSlider
-      label="Arc"
-      defaultValue={[45, 135]}
-      minValue={0}
-      maxValue={360}
-      pageSize={15}
-      onChange={action('onChange')}
-      onChangeEnd={action('onChangeEnd')}
-      showTip
-      formatOptions={{
-        style: 'unit',
-        unit: 'degree',
-        unitDisplay: 'narrow'
-      } as any} />)
-  )
   .add(
     '3 thumbs',
     () => (
diff --git a/packages/@react-spectrum/slider/stories/RangeSlider.stories.tsx b/packages/@react-spectrum/slider/stories/RangeSlider.stories.tsx
@@ -71,10 +71,6 @@ storiesOf('Slider/RangeSlider', module)
   .add(
     'min/max',
     () => render({label: 'Label', minValue: 30, maxValue: 70})
-  )
-  .add(
-    'pageSize',
-    () => render({label: 'Label', minValue: 0, maxValue: 360, pageSize: 15, formatOptions: {style: 'unit', unit: 'degree', unitDisplay: 'narrow'}})
   );
 
 function render(props: SpectrumRangeSliderProps = {}) {
diff --git a/packages/@react-spectrum/slider/stories/Slider.stories.tsx b/packages/@react-spectrum/slider/stories/Slider.stories.tsx
@@ -96,10 +96,6 @@ storiesOf('Slider', module)
     'step',
     () => render({label: 'Label', minValue: 0, maxValue: 100, step: 5})
   )
-  .add(
-    'pageSize',
-    () => render({label: 'Label', minValue: 0, maxValue: 360, pageSize: 15, formatOptions: {style: 'unit', unit: 'degree', unitDisplay: 'narrow'}})
-  )
   .add(
     'isFilled: true',
     () => render({label: 'Label', isFilled: true})
diff --git a/packages/@react-spectrum/slider/test/Slider.test.tsx b/packages/@react-spectrum/slider/test/Slider.test.tsx
@@ -267,16 +267,51 @@ describe('Slider', function () {
 
     it.each`
       Name                          | props                 | commands
-      ${'(left/right arrows, ltr)'} | ${{locale: 'de-DE'}}  | ${[{left: press.ArrowRight, result: +1}, {left: press.ArrowLeft, result: -1}]}
-      ${'(left/right arrows, rtl)'} | ${{locale: 'ar-AE'}}  | ${[{left: press.ArrowRight, result: -1}, {left: press.ArrowLeft, result: +1}]}
+      ${'(left/right arrows, ltr)'} | ${{locale: 'de-DE'}}  | ${[{left: press.ArrowRight, result: +10}, {left: press.ArrowLeft, result: -10}]}
+      ${'(left/right arrows, rtl)'} | ${{locale: 'ar-AE'}}  | ${[{left: press.ArrowRight, result: -10}, {left: press.ArrowLeft, result: +10}]}
       ${'(page up/down, ltr)'}      | ${{locale: 'de-DE'}}  | ${[{left: press.PageUp, result: +20}, {left: press.PageDown, result: -20}]}
       ${'(page up/down, rtl)'}      | ${{locale: 'ar-AE'}}  | ${[{left: press.PageUp, result: +20}, {left: press.PageDown, result: -20}]}
-    `('$Name respects the page size', function ({props, commands}) {
+    `('$Name sets page size to a multiple of step', function ({props, commands}) {
       let tree = render(
         <Provider theme={theme} {...props}>
-          <Slider label="Label" pageSize={20} defaultValue={50} />
+          <Slider label="Label" minValue={0} maxValue={230} defaultValue={50} step={10} />
         </Provider>
       );
+      // The slider page size should be initially calulated as 230/10 = 23 and then snapped to 20 so it is a multiple of the step
+      let slider = tree.getByRole('slider');
+      testKeypresses([slider, slider], commands);
+    });
+
+    it.each`
+      Name                          | props                 | commands
+      ${'(left/right arrows, ltr)'} | ${{locale: 'de-DE'}}  | ${[{left: press.ArrowRight, result: +2}, {left: press.ArrowLeft, result: -2}]}
+      ${'(left/right arrows, rtl)'} | ${{locale: 'ar-AE'}}  | ${[{left: press.ArrowRight, result: -2}, {left: press.ArrowLeft, result: +2}]}
+      ${'(page up/down, ltr)'}      | ${{locale: 'de-DE'}}  | ${[{left: press.PageUp, result: +2}, {left: press.PageDown, result: -2}]}
+      ${'(page up/down, rtl)'}      | ${{locale: 'ar-AE'}}  | ${[{left: press.PageUp, result: +2}, {left: press.PageDown, result: -2}]}
+    `('$Name sets page size to a multiple of step (scenario: step is less than min)', function ({props, commands}) {
+      let tree = render(
+        <Provider theme={theme} {...props}>
+          <Slider label="Label" minValue={50} maxValue={75} defaultValue={60} step={2} />
+        </Provider>
+      );
+      // The slider page size should be initially calulated as 25/10 = 2.5, snaps to 2
+      let slider = tree.getByRole('slider');
+      testKeypresses([slider, slider], commands);
+    });
+
+    it.each`
+    Name                          | props                 | commands
+      ${'(left/right arrows, ltr)'} | ${{locale: 'de-DE'}}  | ${[{left: press.ArrowRight, result: +2}, {left: press.ArrowLeft, result: -2}]}
+      ${'(left/right arrows, rtl)'} | ${{locale: 'ar-AE'}}  | ${[{left: press.ArrowRight, result: -2}, {left: press.ArrowLeft, result: +2}]}
+      ${'(page up/down, ltr)'}      | ${{locale: 'de-DE'}}  | ${[{left: press.PageUp, result: +4}, {left: press.PageDown, result: -4}]}
+      ${'(page up/down, rtl)'}      | ${{locale: 'ar-AE'}}  | ${[{left: press.PageUp, result: +4}, {left: press.PageDown, result: -4}]}
+    `('$Name sets page size to a multiple of step (scenario: step is greater than max)', function ({props, commands}) {
+      let tree = render(
+        <Provider theme={theme} {...props}>
+          <Slider label="Label" minValue={-50} maxValue={-15} defaultValue={-40} step={2} />
+        </Provider>
+      );
+      // The slider page size should be initially calulated as 35/10 = 3.5, snaps to 4
       let slider = tree.getByRole('slider');
       testKeypresses([slider, slider], commands);
     });
diff --git a/packages/@react-stately/color/src/useColorSliderState.ts b/packages/@react-stately/color/src/useColorSliderState.ts
@@ -58,6 +58,7 @@ export function useColorSliderState(props: ColorSliderStateOptions): ColorSlider
     }
   });
 
+  let {step, pageSize} = color.getChannelRange(channel);
   return {
     ...sliderState,
     value: color,
@@ -84,6 +85,8 @@ export function useColorSliderState(props: ColorSliderStateOptions): ColorSlider
     },
     getThumbValueLabel() {
       return color.formatChannelValue(channel, locale);
-    }
+    },
+    step,
+    pageSize
   };
 }
diff --git a/packages/@react-stately/slider/src/useSliderState.ts b/packages/@react-stately/slider/src/useSliderState.ts
@@ -13,7 +13,7 @@
 import {clamp, snapValueToStep} from '@react-aria/utils';
 import {SliderProps} from '@react-types/slider';
 import {useControlledState} from '@react-stately/utils';
-import {useRef, useState} from 'react';
+import {useMemo, useRef, useState} from 'react';
 
 export interface SliderState {
   /**
@@ -159,10 +159,16 @@ export function useSliderState(props: SliderStateOptions): SliderState {
     minValue = DEFAULT_MIN_VALUE,
     maxValue = DEFAULT_MAX_VALUE,
     numberFormatter: formatter,
-    step = DEFAULT_STEP_VALUE,
-    pageSize = Math.max((maxValue - minValue) / 10, step)
+    step = DEFAULT_STEP_VALUE
   } = props;
 
+  // Page step should be at least equal to step and always a multiple of the step.
+  let pageSize = useMemo(() => {
+    let calcPageSize = (maxValue - minValue) / 10;
+    calcPageSize = snapValueToStep(calcPageSize, 0, calcPageSize + step, step);
+    return Math.max(calcPageSize, step);
+  }, [step, maxValue, minValue]);
+
   const [values, setValues] = useControlledState<number[]>(
     props.value as any,
     props.defaultValue ?? [minValue] as any,
diff --git a/packages/@react-types/slider/src/index.d.ts b/packages/@react-types/slider/src/index.d.ts
@@ -39,12 +39,7 @@ export interface SliderProps<T = number[]> extends RangeInputBase<number>, Value
    * The slider's step value.
    * @default 1
    */
-  step?: number,
-  /**
-   * The slider's page step value, used when incrementing and decrementing.
-   * @default 1
-   */
-  pageSize?: number
+  step?: number
 }
 
 export interface SliderThumbProps extends FocusableProps, Validation, LabelableProps {
