-
Notifications
You must be signed in to change notification settings - Fork 5
UIEXT-2930: Handle stepSize setting for number inputs #100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
ef7ca64
9ca81e8
ed6429c
e07d7ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| "@knime/components": patch | ||
| "@knime/jsonforms": patch | ||
| --- | ||
|
|
||
| Make step size of NumberInputWidget configurable |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,6 +42,10 @@ export default { | |
| default: Number.MAX_SAFE_INTEGER, | ||
| type: Number, | ||
| }, | ||
| step: { | ||
| default: null, | ||
| type: Number, | ||
| }, | ||
| /** | ||
| * Validity controlled by the parent component to be flexible. | ||
| */ | ||
|
|
@@ -87,6 +91,10 @@ export default { | |
| return this.type === "integer"; | ||
| }, | ||
| stepSize() { | ||
| if (this.step !== null && this.step !== 0) { | ||
| return this.step; | ||
| } | ||
|
|
||
| return this.isInteger | ||
| ? DEFAULT_STEP_SIZE_INTEGER | ||
| : DEFAULT_STEP_SIZE_DOUBLE; | ||
|
|
@@ -204,7 +212,12 @@ export default { | |
|
|
||
| /** Mimic stepping to nearest step with safe value rounding */ | ||
| let parsedVal = value + increment; | ||
| parsedVal = Math.round(parsedVal * 10) / 10; // eslint-disable-line no-magic-numbers | ||
| let scaleFactor = 1 / Math.abs(increment); | ||
| if (Math.abs(increment) < 1) { | ||
| // Avoid rounding errors induced by fractional increments | ||
| scaleFactor = Math.round(scaleFactor); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if increment < 0.5, then scaleFactor will be rounded to 0, right? Won't we have a division by zero in line 220 then?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not quite. If increment < 0.5, scaleFactor > 2. If increment > 2, scaleFactor < 0.5, but then we wouldn't be in this if block. |
||
| } | ||
| parsedVal = Math.round(parsedVal * scaleFactor) / scaleFactor; | ||
|
|
||
| /** | ||
| * All measures have been taken to ensure a valid value at this point, so if the last | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -15,10 +15,14 @@ const props = defineProps< | |||||
|
|
||||||
| const DEFAULT_STEP_SIZE_INTEGER = 1; | ||||||
| const DEFAULT_STEP_SIZE_DOUBLE = 0.1; | ||||||
| const stepSize = | ||||||
| props.type === "integer" | ||||||
| ? DEFAULT_STEP_SIZE_INTEGER | ||||||
| : DEFAULT_STEP_SIZE_DOUBLE; | ||||||
|
|
||||||
| const stepSize = computed( | ||||||
| () => | ||||||
| props.control.uischema.options?.stepSize ?? | ||||||
| (props.type === "integer" | ||||||
| ? DEFAULT_STEP_SIZE_INTEGER | ||||||
| : DEFAULT_STEP_SIZE_DOUBLE), | ||||||
| ); | ||||||
|
|
||||||
| type BoundValidationParameters = { | ||||||
| isExclusive: boolean; | ||||||
|
|
@@ -61,7 +65,7 @@ const onFocusOut = () => { | |||||
| if (minParams.value && !respectsMin(minParams.value)(comparisonValue)) { | ||||||
| const { min, isExclusive } = minParams.value; | ||||||
| if (isExclusive) { | ||||||
| updatedValue = min + stepSize; | ||||||
| updatedValue = min + stepSize.value; | ||||||
| } else { | ||||||
| updatedValue = min; | ||||||
| } | ||||||
|
|
@@ -71,7 +75,7 @@ const onFocusOut = () => { | |||||
| ) { | ||||||
| const { max, isExclusive } = maxParams.value; | ||||||
| if (isExclusive) { | ||||||
| updatedValue = max - stepSize; | ||||||
| updatedValue = max - stepSize.value; | ||||||
| } else { | ||||||
| updatedValue = max; | ||||||
| } | ||||||
|
|
@@ -91,6 +95,7 @@ const onFocusOut = () => { | |||||
| :type="type" | ||||||
| :min="minParams?.min" | ||||||
| :max="maxParams?.max" | ||||||
| :step="stepSize" | ||||||
|
||||||
| :step="stepSize" | |
| :step="stepSize.value" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was wrong with this old logic / why did we add the scaleFactor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was hardcoded to a maximum precision of 1 decimal place. So a step interval of 0.001 would be treated as 0.1.
But there are other issues in my current implementation as you've pointed out. I'll fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I (and claude) added some unit tests which can double as quasi-documentation of expected behavior.