callstack / react-native-slider

React Native component exposing Slider from iOS and SeekBar from Android
MIT License
1.22k stars 270 forks source link

Fix stepMarked prop of the StepMarker when the step props is not defa… #581

Closed betko closed 5 months ago

betko commented 8 months ago

Summary:

Hi guys, first I would like to thank you the contribution of this library. We are using it in multiple react native applications.

At the last few weeks I am following your new functionality about the custom Step Marker, because right now I should change our application design and this step marker would be the best solution for me. But unfortunately I found out that if I change the default step property (which is 1), my implemented Step Marker is broken.

I check the implementation and I found out that the problem is coming from the option initialisation in the Slider.tsx file

Old:

This initialisation always generates an array with length (max-min / step) + 1 and the values would be [0, 1, 2, ... (max-min / step) + 1]. For the rendering this works correctly but in case of I want to differentiate the selected one from others it starts acting strangely. Please see the attached video.

const options = Array.from(
    {
      length:
        (localProps.maximumValue! - localProps.minimumValue!) /
          (localProps.step
            ? localProps.step
            : constants.DEFAULT_STEP_RESOLUTION) +
        1,
    },
    (_, index) => index,
  );

https://github.com/callstack/react-native-slider/assets/19413170/541eb386-4cf6-44a1-9cb5-3bfb2f6d83f7

New:

With this new initialisation the values of the option parameter will be the value of the steps, because I changed the mapping function which calculates the value based on the step and min value.

  const step = localProps.step
    ? localProps.step
    : constants.DEFAULT_STEP_RESOLUTION;

  const options = Array.from(
    {
      length: (localProps.maximumValue! - localProps.minimumValue!) / step + 1,
    },
    (_, index) => localProps.minimumValue! + index * step,
  );

Note:

Under this Pull request I changed the type of StepMarker props as well, because I would like to know the currentValue inside my custom component. I think this can be something which will be useful for the other users. I hope this doesn't affect any other functionalities.

Test Plan:

https://github.com/callstack/react-native-slider/assets/19413170/a401d659-3d68-4ce5-8fa3-b886415585dd

tgmarinho commented 8 months ago

https://github.com/callstack/react-native-slider/issues/582

BartoszKlonowski commented 6 months ago

@betko Here's the commit that fixes the issue with default vs custom values of min, max and default step:

bartoszklonowski@INV-0029 package % git show 5af5333 
commit 5af53339d4caa4e4e38204c37e543cf6686507b2 (HEAD -> fix/581/default-min-max-step-values, origin/fix/581/default-min-max-step-values)
Author: BartoszKlonowski <Bartosz.Klonowski@callstack.com>
Date:   Fri Apr 26 23:16:58 2024 +0200

    Fix the issue with default vs custom min/max/step props

diff --git a/example/src/Examples.tsx b/example/src/Examples.tsx
index 969407e..495aa81 100644
--- a/example/src/Examples.tsx
+++ b/example/src/Examples.tsx
@@ -275,7 +275,7 @@ const MyStepMarker: FC<MarkerProps> = ({stepMarked, currentValue}) => {
       <View style={styles.separator} />
       <View style={styles.label}>
         {currentValue !== undefined ? (
-          <Text>{currentValue}</Text>
+          <Text>{currentValue % 1 === 0 ? currentValue : currentValue.toFixed(2)}</Text>
         ) : (
           <Text>{'-'}</Text>
         )}
@@ -299,16 +299,36 @@ const SliderExampleWithCustomMarker = (props: SliderProps) => {
     <View>
       <Text style={styles.text}>{value && +value.toFixed(3)}</Text>
       <Slider
-        step={CONSTANTS.STEP}
+        step={1}
+        style={[styles.slider, props.style]}
+        minimumValue={0}
+        maximumValue={13}
+        thumbImage={require('./resources/empty.png')}
+        tapToSeek
+        {...props}
+        value={value}
+        onValueChange={setValue}
+        StepMarker={MyStepMarker}
+        minimumTrackTintColor={'#00629A'}
+        maximumTrackTintColor={'#979EA4'}
+      />
+    </View>
+  );
+};
+
+const SliderExampleWithCustomMarkerWithDefaultProps = (props: SliderProps) => {
+  const [value, setValue] = useState(props.value ?? CONSTANTS.MIN_VALUE);
+
+  return (
+    <View>
+      <Text style={styles.text}>{value && +value.toFixed(3)}</Text>
+      <Slider
         style={[styles.slider, props.style]}
-        minimumValue={CONSTANTS.MIN_VALUE}
-        maximumValue={CONSTANTS.MAX_VALUE}
         thumbImage={require('./resources/empty.png')}
         tapToSeek
         {...props}
         value={value}
         onValueChange={setValue}
-        lowerLimit={1}
         StepMarker={MyStepMarker}
         minimumTrackTintColor={'#00629A'}
         maximumTrackTintColor={'#979EA4'}
@@ -342,7 +362,7 @@ const styles = StyleSheet.create({
   },
   label: {
     marginTop: 10,
-    width: 45,
+    width: 55,
     paddingVertical: 5,
     paddingHorizontal: 10,
     backgroundColor: '#ffffff',
@@ -361,7 +381,7 @@ const styles = StyleSheet.create({
     alignItems: 'center',
   },
   tinyLogo: {
-    marginVertical: 5,
+    marginVertical: 2,
     aspectRatio: 1,
     flex: 1,
     height: '100%',
@@ -608,6 +628,12 @@ export const examples: Props[] = [
       return <SliderExampleWithCustomMarker />;
     },
   },
+  {
+    title: 'Custom step marker but default step and min/max values',
+    render() {
+      return <SliderExampleWithCustomMarkerWithDefaultProps />;
+    },
+  },
   {
     title: 'Inverted slider direction',
     render() {
diff --git a/package/src/Slider.tsx b/package/src/Slider.tsx
index d77f28b..80fd289 100644
--- a/package/src/Slider.tsx
+++ b/package/src/Slider.tsx
@@ -216,15 +216,17 @@ const SliderComponent = (
   );
   const [width, setWidth] = useState(0);

-  const step = localProps.step
+  const stepResolution = localProps.step
     ? localProps.step
     : constants.DEFAULT_STEP_RESOLUTION;

+  const defaultStep = (localProps.maximumValue! - localProps.minimumValue!) / stepResolution;
+  const stepLength = localProps.step || defaultStep;
   const options = Array.from(
     {
-      length: (localProps.maximumValue! - localProps.minimumValue!) / step + 1,
+      length: (localProps.step ? defaultStep : stepResolution) + 1,
     },
-    (_, index) => localProps.minimumValue! + index * step,
+    (_, index) => localProps.minimumValue! + index * stepLength
   );

   const defaultStyle =
diff --git a/package/src/utils/constants.ts b/package/src/utils/constants.ts
index eec4967..3cfb27d 100644
--- a/package/src/utils/constants.ts
+++ b/package/src/utils/constants.ts
@@ -1,8 +1,10 @@
+import { Platform } from "react-native";
+
 export const constants = {
   MARGIN_HORIZONTAL_PADDING: 0.05,
   STEP_NUMBER_TEXT_FONT_SMALL: 8,
   STEP_NUMBER_TEXT_FONT_BIG: 12,
   LIMIT_MIN_VALUE: Number.MIN_SAFE_INTEGER,
   LIMIT_MAX_VALUE: Number.MAX_SAFE_INTEGER,
-  DEFAULT_STEP_RESOLUTION: 100,
+  DEFAULT_STEP_RESOLUTION: Platform.OS === 'android' ? 128 : 1000,
 };

Note: iOS has some problems with the resolution, as it's 10000 steps by default with plenty of digits after the point. So let's have what we have, and can revisit the default resolution on iOS later.

Once you check and apply them to your PR I'm happy to merge it 👍