Closed taeh98 closed 3 years ago
Hi, thanks for your feedback is greatly appreciated. I fixed the issue you have reported using the placeholder as you mentioned and using a debounce event (500ms at the moment but can be change using the new prop debounceTime, the name of the prop 'is not definitive') for trigger the onMax or onMin so the user can type a value with a less value than the min value prop without overwrite it with the min or max value. Now I need to do some others tests before release it on the next distribution version.
OK, thanks! When I first started using this package, I tried to add a placeholder string value since I saw it on the list of props in the documentation but I wasn't able to because of a type conflict (I am using this package in a TypeScript project). I think that it happened because the TypeScript package that provides type definitions for this package (@types/react-native-input-spinner) is out of date. I made a new issue here to add built-in TypeScript support into this package that I think would solve that issue.
Let me know if there are some bugs of all its fine about the main issue.
Thank you very much for your help so far!
I tried running the updated version (1.4.2) but got an error as soon as any page with a spinner was about to open. I was running locally from expo (40.0.0) on an Android device. The only thing I changed was changing the dependency to the updated version (and removing the old TypeScript library). The error was:
Error while updating property "placeholder" in shadow of node of type: AndroidTextInput
java.lang.Double cannot be cast to java.lang.String
Edit: I did some more testing and the error I described above goes away when I provide the placeholder prop to the spinner. If it's needed I think the prop should be made required rather than optional in the type definitions (using "placeholder: string" instead of "placeholder?: string"). Another problem I can see other than that is that the placeholder text (like "Enter value") didn't show when I erased the current value with backspace. Maybe that's a sizing/layout issue. Other than that, the changes seemed to work well for when the min was 0.
I really like the button holding feature too! The only improvement I can think of is to accelerate the speed you go through values when you hold it for a while. That might help when users are going through lots of possible values.
On an input I have where the min is 1 (1.0), after a small delay after I backspace, the value goes back to 1 or appends or replaces the current value with 1 if I type another value in before this happens. You mentioned you wanted a delay (the debounce) - is this a time after which the value goes to the min value if the user hasn't typed anything? It maybe makes more sense to keep the value blank (instead of showing the value, showing the string placeholder prop) until the user types something themselves rather than going to the minimum value after a delay. This is because if the value has been erased the user has to be using the keyboard to have erased it with backspace and is about to input their own value anyway. Even aside from the current issues, maybe this is a better approach. Thanks again!
I checked and there was some issues related to the placeholder. Fixed on 1.4.3. About the increase on long press speed after holding the button, yes, I was thinking about the same feature.
Thanks for your help again! I took a look at the update (1.4.3) and it seems like everything works when min = 0, but the problems I talked about before when min = 1 are still there. After using backspace to erase the value, the placeholder is shown in grey. For some reason, though, after a small pause, the value goes back to 1 for some reason (even if you have typed a different value beforehand). This stops the user from inputting the value they want to with a keyboard.
Also, I made a gist that roughly outlined how accelerating the speed values change while a button is held could work: https://gist.github.com/taeh98/f709451457400818094d802cd33694d5
If you control it by setting speed rather than change intervals and it'd be useful please let me know and I could refactor it on that basis if you'd like? I'd contribute this feature myself but I wouldn't be sure where it fits in.
Thanks for your help again! I took a look at the update (1.4.3) and it seems like everything works when min = 0, but the problems I talked about before when min = 1 are still there. After using backspace to erase the value, the placeholder is shown in grey. For some reason, though, after a small pause, the value goes back to 1 for some reason (even if you have typed a different value beforehand). This stops the user from inputting the value they want to with a keyboard.
Check on last release 1.4.4 it should be fixed
Also, I made a gist that roughly outlined how accelerating the speed values change while a button is held could work: https://gist.github.com/taeh98/f709451457400818094d802cd33694d5
If you control it by setting speed rather than change intervals and it'd be useful please let me know and I could refactor it on that basis if you'd like? I'd contribute this feature myself but I wouldn't be sure where it fits in.
Thanks for that, I will implement the features using that solution
Check on last release 1.4.4 it should be fixed
I just tried it out with the updated version and it still happens. When I erase the current value with backspace out when the min is 1, it keeps resetting it back to 1 (after a small pause after erasing it with backspace).
I used the following code, Expo 40.0.0, version 1.4.4 of this package and was running locally via npm on an Android device: <AbstractNumericInput step={1} max={10} min={1} value={this.state.val} onChange={(val: number) => this.setState(({val: val}))} /> I ran this code in the root App component with the only other code as the state holding val as a number to check this issue was still present.
Thanks for that, I will implement the features using that solution
No worries! It's the least I could do after all your time fixing my issues :)
I just tried it out with the updated version and it still happens. When I erase the current value with backspace out when the min is 1, it keeps resetting it back to 1 (after a small pause after erasing it with backspace).
So you asking about that the input can be totally empty returning empty value on the onChange
event?
At the moment is working that if the value is empty, it is like the min number, so is showed as placeholder the min number and returned on the onChange
event the min number instead the empty value.
So you asking about that the input can be totally empty returning empty value on the
onChange
event?
At the moment, when you use backspace to erase the current value, spinners with min = 0 and min = 1 behave differently, and I think those with min = 1 should be like those with min = 0. When min = 0, if you erase a value it shows the placeholder (text or the min) in grey and lets you enter a value. But when it's 1, it shows the placeholder (in grey) for a small time before it then makes min the current value (in black). Because this is inconsistent with min = 0 and stops users entering the values they want to, I don't think this should happen - spinners with min = 1 should be made to behave like those with min = 0 do now.
If you want to see what I mean, try copying and pasting <AbstractNumericInput step={1} max={10} min={1} value={this.state.val} onChange={(val: number) => this.setState(({val: val}))} /> into App.js or App.tsx to see how the spinner behaves when you use backspace to erase its value when min = 0 and when min = 1; you will see the different behavior I'm talking about.
I think this would mean that the onChange event might be an undefined/null value when the user erases it, but this already happens when the user erases the value anyway. I just don't think that afterwards the min value should be filled in automatically since the user must be about to type in a value anyway (since they can only erase the value using the keyboard to begin with).
At the moment is working that if the value is empty, it is like the min number, so is showed as placeholder the min number and returned on the
onChange
event the min number instead the empty value.
OK so I think that you check if the value is empty and if so you replace it with the min? But I don't understand why spinners with min = 0 and min = 1 would behave differently in that case :/
So you asking about that the input can be totally empty returning empty value on the
onChange
event?At the moment, when you use backspace to erase the current value, spinners with min = 0 and min = 1 behave differently, and I think those with min = 1 should be like those with min = 0. When min = 0, if you erase a value it shows the placeholder (text or the min) in grey and lets you enter a value. But when it's 1, it shows the placeholder (in grey) for a small time before it then makes min the current value (in black). Because this is inconsistent with min = 0 and stops users entering the values they want to, I don't think this should happen - spinners with min = 1 should be made to behave like those with min = 0 do now.
If you want to see what I mean, try copying and pasting <AbstractNumericInput step={1} max={10} min={1} value={this.state.val} onChange={(val: number) => this.setState(({val: val}))} /> into App.js or App.tsx to see how the spinner behaves when you use backspace to erase its value when min = 0 and when min = 1; you will see the different behavior I'm talking about.
I think this would mean that the onChange event might be an undefined/null value when the user erases it, but this already happens when the user erases the value anyway. I just don't think that afterwards the min value should be filled in automatically since the user must be about to type in a value anyway (since they can only erase the value using the keyboard to begin with).
At the moment is working that if the value is empty, it is like the min number, so is showed as placeholder the min number and returned on the
onChange
event the min number instead the empty value.OK so I think that you check if the value is empty and if so you replace it with the min? But I don't understand why spinners with min = 0 and min = 1 would behave differently in that case :/
I saw that issue, solved on 1.4.5 and about empty value I've added a new prop for do that emptied
maybe could be useful in some cases.
I saw that issue, solved on 1.4.5 and about empty value I've added a new prop for do that
emptied
maybe could be useful in some cases.
The problems are, unfortunately, still there; I tried running the update and 1 is still reappearing a small time after erasing the current value when min = 1 but not when min = 0. I tested this with the following code:
<AbstractNumericInput step={1} max={10} min={0} value={this.state.val} onChange={(val: number) => this.setState(({val: val}))} />
I think there are also some problems with the accelerating cycle speed when holding buttons, I'll see if I can help somewhere :) Do you think it could also be good to restore the erased value if a user erases the current value and they then stop typing and close the keyboard to do something else?
I'm going to close this issue to make more specific ones to the issues that remain after you have fixed the original ones.
When you use backspace to erase the current value and the minimum (min prop) is negative or less than 0, a value of 0 appears (perhaps as a placeholder or because of an undefined value) that itself cannot be erased: when you do it reappears. Though you can type to overwrite it, its presence (as opposed to a blank input) could be confusing to users and degrade usability.
Further (and more importantly), when the minimum value is greater than 0, when you use backspace to erase the current value the minimum value is placed as the new value instead of 0 as the "placeholder". The further issue comes as even then when the user types an input, it is not overwritten, only appended. For example, if the minimum value was 1 and the user used backspace to erase the current value, 1 would then be present. If they then typed 2, this would append 1 as opposed to overwriting it: making 12 instead of 2, as the user intends. This means that if the maximum is under 12 in this example, the user has to repeatedly press the plus button to increment the value until it is at their desired value instead of being able to simply type it. This could severely degrade UX with precise inputs with small step values or wide ranges.
Please could you fix these issues by implementing changes to ensure these issues no longer occur? Perhaps instead of showing the minimum or a default value use a grey text placeholder like "Enter value" after the user uses backspace to erase their current value.
I tested these issues with Expo running locally and with Expo APK builds on Android with the min prop set to -1, 0, 1, and 2.