fix: trim increment number value #490
Conversation
| @@ -91,7 +91,7 @@ function NumberArrowStep(props) { | |||
| return value; | |||
| }; | |||
|
|
|||
| const setValue = (value) => editField(field, [ 'increment' ], value); | |||
| const setValue = (value) => editField(field, [ 'increment' ], value && Big(value).toString()); | |||
There was a problem hiding this comment.
It seems like this is not side-effect free. When typing rather low and starting to type the digits, the . got erased rather quickly.
There was a problem hiding this comment.
Very nice catch, there are three ways we can fix this if we want to keep trimming
[1] We don't treat 45. as a valid value, and wait for the user to enter a more complete entry:
4 is entered => 4 is serialized
5 is entered => 45 is serialized
. is entered => nothing happens
5 is entered => 45.5 is serialized
The downside of this is, if someone were to enter 45. quickly, because the field is debounced, we may have no value at all as the two first steps mentioned above may be outright skipped.
[2] We serialize the dot in the schema
This avoids the above mentioned downside, but may be a little weird in terms of schema to store something like 23.
[3] We implement something at the properties panel field level to deal with this exact scenario
Downside here is implementation time, I'm not sure how/if such an implementation would work well.
@christian-konrad @pinussilvestrus maybe you have some input here? Personally I think option 2 is the right call.
There was a problem hiding this comment.
We serialize the dot in the schema
As far as I can see45. is valid input data for the numbers itself, as it would simply resolve to 45. You can also specify this via input data; no error is thrown.
Therefore we would be fine persisting it this way in the schema for the increment, as it would be consistent. If we can avoid trimming for such cases, even better - no magic.
There was a problem hiding this comment.
Sounds good to me, I've implemented just that and added a new test case for it.
Skaiir
force-pushed
the
mdlr-3382-trim-increment
branch
from
7a8a2f3
to
e9995aa
Compare
There was a problem hiding this comment.
For the decimal inputs config still accepts non-integer numbers, which I guess it doesn't make sense and it's the point of this PR, no?
Another thing that I think it's unrelated to this PR is that the range validation seems to accept non-integer values but then the validation looks like this
In the image above I used 5.3 as a maximum value
Actually the config doesn't accept non-integer numbers in either cases. The field doesn't throw a validation error when it's wrong, but if you navigate to another component and come back you'll see the decimals aren't serialized. Unfortunately the properties panel number field just doesn't support this, or the decimal step has to be defined explicitly. And since it also uses the browser default decimal which is a bit inconsistent across browsers we can't do much about it, unless we implement our own number field from a non-decimal input field in the properties panel, like we've done in I've raised the issue for min/max here and will tackle it soon, we can fix this by setting a step of For increment, the behavior is correct but the UX is quite bad as it's unclear that an integer is expected. Maybe I should migrate over to a text field with number validation, but then we would lose the increment arrows. But I guess it's not a huge deal. If we want to keep the arrows but also get nice validation, we'll need to make some changes to the properties panel library. I've raise an improvement request here to allow validation of number fields the same way we have them for text fields. |
Skaiir
force-pushed
the
mdlr-3382-trim-increment
branch
from
e9995aa
to
60f9c76
Compare
|
Let's keep the trimming very simple, clearing leading zeroes for As far as camunda/camunda-modeler#3382 second part is concerned, let's not tackle this until we have the ability to define custom validation for the properties panel number fields. For now UX will not be ideal but to be honest this has always been the case with number properties. |
Skaiir
force-pushed
the
mdlr-3382-trim-increment
branch
2 times, most recently
from
6256992
to
1f48164
Compare
packages/form-js-editor/src/features/properties-panel/entries/NumberEntries.js
Outdated
Show resolved
Hide resolved
Skaiir
force-pushed
the
mdlr-3382-trim-increment
branch
from
1f48164
to
04fcf84
Compare
| const clearLeadingZeroes = (value) => { | ||
| if (!value) return value; | ||
| const trimmed = value.replace(/^0+/g, ''); | ||
| return (trimmed.startsWith('.') ? '0' : '') + trimmed; |
There was a problem hiding this comment.
| return (trimmed.startsWith('.') ? '0' : '') + trimmed; | |
| return `${trimmed.startsWith('.') ? '0' : ''}${trimmed}`; |
There was a problem hiding this comment.
I think this is personal preference, but personally, I wouldn't use computed strings for concatenation. So I'll merge as-is. Thanks for the review :)
Related to camunda/camunda-modeler#3382