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

betko · 2024-03-04T15:50:04Z

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,
  );

slider-with-wrong-initialization.mp4

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:

I updated the code of the Example project. I added a new Example where I created a separated StepMarker. Please see the short video about this
I added an new test case (But to be honest I am not sure that it's calculates as a test plan)

slider-new-example.mp4

tgmarinho · 2024-03-11T23:59:17Z

#582

BartoszKlonowski

I'm sorry for such a delay, but I went through and I only have one remark (except for some minor nitpicks) and it should be good to go 👍

BartoszKlonowski · 2024-03-26T19:13:01Z

package/src/components/TrackMark.tsx

 };

 export const SliderTrackMark = ({
  isTrue,
  thumbImage,
  StepMarker,
+  currentValue,


Yes, I was thinking that at some point the current value should be added, so that it's possible to know where the thumb is from the level of StepMarker. I'm glad it also helps to fix this issue.

BartoszKlonowski · 2024-03-26T19:14:13Z

package/src/Slider.tsx

-            ? localProps.step
-            : constants.DEFAULT_STEP_RESOLUTION) +
-        1,
+      length: (localProps.maximumValue! - localProps.minimumValue!) / step + 1,


This is the only thing that bothers me in this PR: What if minimumValue or maximumValue are not given? We have the default ones, maybe we should use those defaults here?

I have to ask what do you mean under the default minimumValue and maximumValue? The value which are defined under the SliderWithRef.defaultProps ?

I removed my defined max and min values and if I defined the step as 1 the custom marker works as expected.

Code from the modified Example code:

const SliderExampleWithCustomMarker = (props: SliderProps) => { const [value, setValue] = useState(props.value ?? CONSTANTS.MIN_VALUE); return ( <View> <Text style={styles.text}>{value && +value.toFixed(3)}</Text> <Slider step={1} style={[styles.slider, props.style]} thumbImage={require('./resources/slider-left.png')} renderStepNumber tapToSeek {...props} value={value} onValueChange={setValue} StepMarker={MyStepMarker} minimumTrackTintColor={'#00629A'} maximumTrackTintColor={'#979EA4'} /> </View> ); };

However unfortunately if the step is the default one (100 as DEFAULT_STEP_RESOLUTION) my logic is broken. Also if the step is for example 0.2 because the division. Do you have any suggestion how we can resolve that?

I think that is because of the value map calculation here:
(_, index) => localProps.minimumValue! + index * step done in line 227.
Thing (means: issue root cause) here is that when having the default step it defaults to 0. So in the end, in line 227, we get minimumValue + 100 for each step.
I don't want to modify the 0 as default step value, but we can try to adjust that according to the default resolution.

@betko ☝️ (forgot to mention/ping)

example/src/Examples.tsx

…ult 1

BartoszKlonowski · 2024-04-26T21:30:51Z

@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 👍

BartoszKlonowski

I think I will just merge it, as it only requires some additional adjustments.
Let's have it and we can take it from there 👍
Thank you, @betko

betko force-pushed the main branch from 5f009bc to bf63c50 Compare March 5, 2024 07:17

BartoszKlonowski reviewed Mar 26, 2024

View reviewed changes

BartoszKlonowski mentioned this pull request Mar 29, 2024

Feature: Custom steps marker #555

Merged

betko force-pushed the main branch from bf63c50 to 856f2ec Compare April 3, 2024 11:57

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

61c17d2

…ult 1

betko force-pushed the main branch from 856f2ec to 61c17d2 Compare April 3, 2024 12:01

BartoszKlonowski mentioned this pull request Jun 11, 2024

Fix the issue with default min/max values with default step for custom StepMarker #606

Closed

BartoszKlonowski approved these changes Jun 11, 2024

View reviewed changes

BartoszKlonowski merged commit c687a95 into callstack:main Jun 11, 2024
8 checks passed

This was referenced Sep 10, 2024

Fix the step resolution calculation for StepMarker and steps when default settings #636

Merged

added value prop to StepMarker component prop #598

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

betko commented Mar 4, 2024

tgmarinho commented Mar 11, 2024

BartoszKlonowski left a comment

BartoszKlonowski Mar 26, 2024

BartoszKlonowski Mar 26, 2024

betko Apr 3, 2024

BartoszKlonowski Apr 26, 2024 •

edited

Loading

BartoszKlonowski Apr 26, 2024

BartoszKlonowski commented Apr 26, 2024

BartoszKlonowski left a comment

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

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

Conversation

betko commented Mar 4, 2024

Summary:

Old:

New:

Test Plan:

tgmarinho commented Mar 11, 2024

BartoszKlonowski left a comment

Choose a reason for hiding this comment

BartoszKlonowski Mar 26, 2024

Choose a reason for hiding this comment

BartoszKlonowski Mar 26, 2024

Choose a reason for hiding this comment

betko Apr 3, 2024

Choose a reason for hiding this comment

BartoszKlonowski Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

BartoszKlonowski Apr 26, 2024

Choose a reason for hiding this comment

BartoszKlonowski commented Apr 26, 2024

BartoszKlonowski left a comment

Choose a reason for hiding this comment

BartoszKlonowski Apr 26, 2024 •

edited

Loading