react/fix inputNumber handleAdjust logic #518

clairesunstudio · 2019-03-26T15:23:07Z

Fixed

(React) [InputCurrency, InputNumber] DP-13167: Fixed handleAdjust logic so that min/max are not required for up/down buttons to work. react/fix inputNumber handleAdjust logic #518
(React) [InputNumber] DP-13167: Fixed the initial steps when using up/down without a default value. react/fix inputNumber handleAdjust logic #518

To test:

In InputCurrency, InputNumber stories, remove min and max props and click the up/down buttons.
In InputNumber story, set step to 0.1 and use up/down button.

react/src/components/atoms/forms/InputNumber/index.js

Co-Authored-By: clairesunstudio <[email protected]>

react/src/components/atoms/forms/InputNumber/index.js

avrilpearl

@clairesunstudio --> this looks good to me, obviously made a bunch of updates to the previous logic so feel free to merge it once you review those.

changelogs/DP-13167.md

clairesunstudio · 2019-03-29T15:03:04Z

changes looks good to me

one small thing i noticed that in InputCurrency, if you put in a dollar value becomes 0, vs if you put in any other invalid character value becomes ""

react/src/components/atoms/forms/InputCurrency/index.js

smurrayatwork · 2019-03-29T15:32:38Z

react/src/components/atoms/forms/InputCurrency/index.js

          const handleBlur = () => {
            const inputEl = ref.current;
-            if (is.empty(inputEl.value)) {
+            const stringValue = inputEl.value;
+            const isNotNumber = !stringValue || isNaN(Number(numbro.unformat(stringValue)));


Suggested change

const isNotNumber = !stringValue || isNaN(Number(numbro.unformat(stringValue)));

const isNotNumber = is.empty(stringValue) || Number.isNaN(Number(numbro.unformat(stringValue)));

i think this is doing the same thing

because stringValue can't be number 0

Number.isNaN() is the same as isNaN() since we already made the parameter a number

react/src/components/atoms/forms/InputCurrency/index.js

smurrayatwork · 2019-03-29T15:39:20Z

react/src/components/atoms/forms/InputNumber/index.js

-                newValue = props.min;
-              }
+            let newValue = inputEl.value ? Number(inputEl.value) : inputEl.value;
+            if (hasNumberProperty(props, 'max') && newValue > props.max) {


Suggested change

if (hasNumberProperty(props, 'max') && newValue > props.max) {

if (hasNumberProperty(props, 'max') && (is.number(newValue) && newValue > props.max)) {

this check is not needed, you can only enter numbers, periods, commas, or null in an input type number, so this will be a number null, or a string of . or , which will evaluate to false above.

smurrayatwork · 2019-03-29T15:39:55Z

react/src/components/atoms/forms/InputNumber/index.js

-            if (!is.empty(inputEl.value)) {
-              inputEl.value = Number(numbro(newValue)
-                .format({ mantissa: countDecimals(props.step) }));
+            if (hasNumberProperty(props, 'min') && newValue < props.min) {


Suggested change

if (hasNumberProperty(props, 'min') && newValue < props.min) {

if (hasNumberProperty(props, 'min') && (is.number(newValue) && newValue < props.min)) {

this check is not needed, you can only enter numbers, periods, commas, or null in an input type number, so this will be a number null, or a string of . or , which will evaluate to false above.

smurrayatwork · 2019-03-29T15:41:27Z

react/src/components/atoms/forms/InputNumber/index.js

-              }
-              handleChange(e);
+            let newValue = inputEl.value ? Number(inputEl.value) : inputEl.value;
+            if (direction === 'up' && (!hasNumberProperty(props, 'max') || newValue < props.max)) {


Suggested change

if (direction === 'up' && (!hasNumberProperty(props, 'max') || newValue < props.max)) {

if (direction === 'up' && (!hasNumberProperty(props, 'max') || (is.number(newValue) && newValue < props.max))) {

this check is not needed, you can only enter numbers, periods, commas, or null in an input type number, so this will be a number, null, or a string of . or , - null and strings will evaluate to false above which is correct so this is fine.

smurrayatwork · 2019-03-29T15:42:51Z

react/src/components/atoms/forms/InputNumber/index.js

-              handleChange(e);
+            let newValue = inputEl.value ? Number(inputEl.value) : inputEl.value;
+            if (direction === 'up' && (!hasNumberProperty(props, 'max') || newValue < props.max)) {
+              newValue = newValue ? (newValue + props.step).toFixed(countDecimals(props.step)) : props.step;


if newValue is a string, toFixed won't work because it doesn't exist.

Suggested change

newValue = newValue ? (newValue + props.step).toFixed(countDecimals(props.step)) : props.step;

newValue = newValue ? Number(newValue + props.step).toFixed(countDecimals(props.step)) : props.step;

newvalue would already be a number at this point - we do not need to cast it again. --> newValue < props.max would only evaluate to true if is was a number less than min prop

smurrayatwork · 2019-03-29T15:43:24Z

react/src/components/atoms/forms/InputNumber/index.js

+                  props.onChange(e, newValue, props.id);
+                }
+              });
+            } else if (direction === 'down' && (!hasNumberProperty(props, 'min') || newValue > props.min)) {


Suggested change

} else if (direction === 'down' && (!hasNumberProperty(props, 'min') || newValue > props.min)) {

} else if (direction === 'down' && (!hasNumberProperty(props, 'min') || (is.number(newValue) && newValue > props.min))) {

this check is not needed, you can only enter numbers, periods, commas, or null in an input type number, so this will be a number, null, or a string of . or , - null and strings will evaluate to false above which is correct so this is fine.

smurrayatwork · 2019-03-29T15:43:38Z

react/src/components/atoms/forms/InputNumber/index.js

+                }
+              });
+            } else if (direction === 'down' && (!hasNumberProperty(props, 'min') || newValue > props.min)) {
+              newValue = newValue ? (newValue + props.step * -1).toFixed(countDecimals(props.step)) : (props.step * -1);


Suggested change

newValue = newValue ? (newValue + props.step * -1).toFixed(countDecimals(props.step)) : (props.step * -1);

newValue = newValue ? Number(newValue + props.step * -1).toFixed(countDecimals(props.step)) : (props.step * -1);

newvalue would already be a number at this point - we do not need to cast it again. --> newValue > props.min would only evaluate to true if is was a number greater than min prop

Co-Authored-By: clairesunstudio <[email protected]>

react/src/components/atoms/forms/InputCurrency/index.js

Co-Authored-By: avrilpearl <[email protected]>

clairesunstudio added 4 commits March 26, 2019 11:18

fix inputNumber handleAdjust logic

c94d3e2

fix input currency min/max logic

ac6a0fc

refactor

19fc2ed

add changelog

ec4491f

clairesunstudio requested review from smurrayatwork and avrilpearl and removed request for smurrayatwork March 26, 2019 18:26

clairesunstudio changed the title ~~fix inputNumber handleAdjust logic~~ react/fix inputNumber handleAdjust logic Mar 26, 2019

clairesunstudio requested a review from smurrayatwork March 26, 2019 18:27

clairesunstudio added the internal review label Mar 26, 2019

smurrayatwork reviewed Mar 26, 2019

View reviewed changes

react/src/components/atoms/forms/InputNumber/index.js Outdated Show resolved Hide resolved

smurrayatwork and others added 3 commits March 26, 2019 16:23

Update react/src/components/atoms/forms/InputNumber/index.js

6fc1e03

Co-Authored-By: clairesunstudio <[email protected]>

simplify input number handle change logic

c6d244f

simply handle blur and handle adjust on input number

560b2d7

smurrayatwork reviewed Mar 28, 2019

View reviewed changes

react/src/components/atoms/forms/InputNumber/index.js Show resolved Hide resolved

smurrayatwork reviewed Mar 28, 2019

View reviewed changes

react/src/components/atoms/forms/InputNumber/index.js Outdated Show resolved Hide resolved

smurrayatwork reviewed Mar 28, 2019

View reviewed changes

react/src/components/atoms/forms/InputNumber/index.js Outdated Show resolved Hide resolved

smurrayatwork approved these changes Mar 28, 2019

View reviewed changes

avrilpearl added 2 commits March 28, 2019 17:18

wip

5b931aa

update input currency

d93df58

avrilpearl reviewed Mar 29, 2019

View reviewed changes

clairesunstudio commented Mar 29, 2019

View reviewed changes

changelogs/DP-13167.md Outdated Show resolved Hide resolved

Update changelogs/DP-13167.md

1cba9f9

smurrayatwork reviewed Mar 29, 2019

View reviewed changes

react/src/components/atoms/forms/InputCurrency/index.js Outdated Show resolved Hide resolved

smurrayatwork reviewed Mar 29, 2019

View reviewed changes

react/src/components/atoms/forms/InputCurrency/index.js Outdated Show resolved Hide resolved

smurrayatwork reviewed Mar 29, 2019

View reviewed changes

react/src/components/atoms/forms/InputCurrency/index.js Show resolved Hide resolved

smurrayatwork reviewed Mar 29, 2019

View reviewed changes

react/src/components/atoms/forms/InputCurrency/index.js Show resolved Hide resolved

smurrayatwork reviewed Mar 29, 2019

View reviewed changes

react/src/components/atoms/forms/InputCurrency/index.js Show resolved Hide resolved

smurrayatwork reviewed Mar 29, 2019

View reviewed changes

Update react/src/components/atoms/forms/InputCurrency/index.js

34d8a1d

Co-Authored-By: clairesunstudio <[email protected]>

clairesunstudio commented Mar 29, 2019

View reviewed changes

react/src/components/atoms/forms/InputCurrency/index.js Outdated Show resolved Hide resolved

Update react/src/components/atoms/forms/InputCurrency/index.js

6660ae8

clairesunstudio commented Mar 29, 2019

View reviewed changes

react/src/components/atoms/forms/InputCurrency/index.js Show resolved Hide resolved

clairesunstudio commented Mar 29, 2019

View reviewed changes

react/src/components/atoms/forms/InputCurrency/index.js Outdated Show resolved Hide resolved

clairesunstudio and others added 3 commits March 29, 2019 14:03

Update react/src/components/atoms/forms/InputCurrency/index.js

f57dadc

Update react/src/components/atoms/forms/InputCurrency/index.js

82edafc

Co-Authored-By: avrilpearl <[email protected]>

cast to fixed back to number

5a64039

clairesunstudio merged commit 15ea3e9 into develop Apr 1, 2019

clairesunstudio deleted the react/input-buttons-bug-fix branch April 1, 2019 15:35

This was referenced Apr 10, 2019

Mayflower 9.4.1 Release #531

Merged

Sync Develop With Master for Release 9.4.1 #532

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

react/fix inputNumber handleAdjust logic #518

react/fix inputNumber handleAdjust logic #518

clairesunstudio commented Mar 26, 2019 •

edited

Loading

avrilpearl left a comment

clairesunstudio commented Mar 29, 2019

smurrayatwork Mar 29, 2019 •

edited

Loading

clairesunstudio Mar 29, 2019

clairesunstudio Mar 29, 2019

clairesunstudio Mar 29, 2019

smurrayatwork Mar 29, 2019

This comment was marked as outdated.

avrilpearl Mar 29, 2019

smurrayatwork Mar 29, 2019

This comment was marked as outdated.

avrilpearl Mar 29, 2019

smurrayatwork Mar 29, 2019

This comment was marked as outdated.

avrilpearl Mar 29, 2019

smurrayatwork Mar 29, 2019

This comment was marked as outdated.

This comment was marked as outdated.

avrilpearl Mar 29, 2019 •

edited

Loading

smurrayatwork Mar 29, 2019

avrilpearl Mar 29, 2019

smurrayatwork Mar 29, 2019

This comment was marked as outdated.

avrilpearl Mar 29, 2019 •

edited

Loading

	const isNotNumber = !stringValue \|\| isNaN(Number(numbro.unformat(stringValue)));
	const isNotNumber = is.empty(stringValue) \|\| Number.isNaN(Number(numbro.unformat(stringValue)));

	if (hasNumberProperty(props, 'max') && newValue > props.max) {
	if (hasNumberProperty(props, 'max') && (is.number(newValue) && newValue > props.max)) {

	if (hasNumberProperty(props, 'min') && newValue < props.min) {
	if (hasNumberProperty(props, 'min') && (is.number(newValue) && newValue < props.min)) {

	if (direction === 'up' && (!hasNumberProperty(props, 'max') \|\| newValue < props.max)) {
	if (direction === 'up' && (!hasNumberProperty(props, 'max') \|\| (is.number(newValue) && newValue < props.max))) {

	newValue = newValue ? (newValue + props.step).toFixed(countDecimals(props.step)) : props.step;
	newValue = newValue ? Number(newValue + props.step).toFixed(countDecimals(props.step)) : props.step;

	} else if (direction === 'down' && (!hasNumberProperty(props, 'min') \|\| newValue > props.min)) {
	} else if (direction === 'down' && (!hasNumberProperty(props, 'min') \|\| (is.number(newValue) && newValue > props.min))) {

react/fix inputNumber handleAdjust logic #518

react/fix inputNumber handleAdjust logic #518

Conversation

clairesunstudio commented Mar 26, 2019 • edited Loading

avrilpearl left a comment

Choose a reason for hiding this comment

clairesunstudio commented Mar 29, 2019

smurrayatwork Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

avrilpearl Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

avrilpearl Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

clairesunstudio commented Mar 26, 2019 •

edited

Loading

smurrayatwork Mar 29, 2019 •

edited

Loading

avrilpearl Mar 29, 2019 •

edited

Loading

avrilpearl Mar 29, 2019 •

edited

Loading