Skip to content
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

react/fix inputNumber handleAdjust logic #518

Merged
merged 15 commits into from
Apr 1, 2019
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions changelogs/DP-13167.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Patch
Fixed
- (React) [InputCurrency, InputNumber] DP-13167: Fixed handleAdjust logic so that min/max are not required for up/down buttons to work. #518
- (React) [InputNumber] DP-13167: Fixed the initial steps when using up/down without a default value and decimal formatting onBlur. #518
151 changes: 67 additions & 84 deletions react/src/components/atoms/forms/InputCurrency/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,148 +47,130 @@ const Currency = (props) => {
}
return number;
};
const hasProperty = (obj, property) => Object.prototype.hasOwnProperty.call(obj, property) && !is.nil(obj[property]);
const hasNumberProperty = (obj, property) => Object.prototype.hasOwnProperty.call(obj, property) && is.number(obj[property]);
const greaterThanMin = (val) => !hasNumberProperty(props, 'min') || (val >= props.min);
const lessThanMax = (val) => !hasNumberProperty(props, 'max') || (val <= props.max);

const displayErrorMessage = (val) => {
const { min, max, required } = props;
if (required && !is.number(val)) {
const errorMsg = 'Please enter a value.';
return{
showError: true,
errorMsg
};
} else if (is.number(val)) {
const { showError, errorMsg } = validNumber(val, min, max);
return{
showError, errorMsg
};
}
return{
showError: false,
errorMsg: ''
};
};

const handleChange = (e) => {
const { type } = e;
const stringValue = ref.current.value;
let numberValue;
const update = {
value: stringValue
};
if (is.empty(stringValue)) {
numberValue = 0;
} else {
numberValue = Number(numbro.unformat(stringValue));
}
// This validation is needed here as onKeyDown does not
// get the new value in the input after a key press.
if (props.required && is.empty(stringValue)) {
errorMsg = 'Please enter a value.';
update.showError = true;
update.errorMsg = errorMsg;
} else if (is.number(numberValue) && !is.empty(stringValue)) {
const validate = validNumber(numberValue, props.min, props.max);
update.showError = validate.showError;
update.errorMsg = validate.errorMsg;
} else {
errorMsg = '';
update.showError = false;
update.errorMsg = errorMsg;
}
context.updateState(update, () => {
if (typeof props.onChange === 'function') {
const numberValue = stringValue ? Number(numbro.unformat(ref.current.value)) : 0;
clairesunstudio marked this conversation as resolved.
Show resolved Hide resolved
// If the stringvalue is empty, set to empty string so the required error
// message is rendered. Otherwise pass the number value for the min/max check.
const updateError = displayErrorMessage(!is.empty(stringValue) ? numberValue : '');
context.updateState({ value: stringValue, ...updateError }, () => {
if (is.fn(props.onChange)) {
props.onChange(numberValue, props.id, type);
}
});
};

const handleAdjust = (e) => {
const direction = (e.currentTarget === upRef.current) ? 'up' : 'down';
const { type } = e;
let numberValue;
const stringValue = ref.current.value;
if (is.empty(stringValue)) {
numberValue = 0;
} else {
numberValue = Number(numbro.unformat(ref.current.value));
const numberValue = stringValue ? Number(numbro.unformat(ref.current.value)) : 0;
clairesunstudio marked this conversation as resolved.
Show resolved Hide resolved
clairesunstudio marked this conversation as resolved.
Show resolved Hide resolved
let newValue;
if (direction === 'up') {
newValue = Number(numbro(numberValue).add(props.step).format({ mantissa: countDecimals(props.step) }));
} else if (direction === 'down') {
newValue = Number(numbro(numberValue).subtract(props.step).format({ mantissa: countDecimals(props.step) }));
}
if (is.number(numberValue)) {
let newValue;
if (direction === 'up') {
newValue = Number(numbro(numberValue).add(props.step).format({ mantissa: countDecimals(props.step) }));
} else if (direction === 'down') {
newValue = Number(numbro(numberValue).subtract(props.step).format({ mantissa: countDecimals(props.step) }));
}
if ((!hasProperty(props, 'min') || newValue >= props.min) && (!hasProperty(props, 'max') || (newValue <= props.max))) {
const { showError, errorMsg } = validNumber(newValue, props.min, props.max);
context.updateState({
showError,
errorMsg,
value: toCurrency(newValue, countDecimals(props.step))
}, () => {
if (typeof props.onChange === 'function') {
props.onChange(newValue, props.id, type, direction);
}
});
}
if (greaterThanMin(newValue) && lessThanMax(newValue)) {
const updateError = displayErrorMessage(!is.empty(stringValue) ? newValue : '');
context.updateState({ value: toCurrency(newValue, countDecimals(props.step)), ...updateError }, () => {
if (is.fn(props.onChange)) {
props.onChange(newValue, props.id, type, direction);
}
});
}
};

const handleKeyDown = (e) => {
const { type, key } = e;
const stringValue = ref.current.value;
let numberValue;
if (is.empty(stringValue)) {
numberValue = 0;
} else {
numberValue = Number(numbro.unformat(stringValue));
}
const numberValue = stringValue ? Number(numbro.unformat(stringValue)) : 0;
clairesunstudio marked this conversation as resolved.
Show resolved Hide resolved
// default to 0 if defaultValue is NaN
if (is.number(numberValue) && !is.empty(stringValue)) {
let newValue = numberValue;
if (key === 'ArrowDown') {
newValue = Number(numbro(numberValue).subtract(props.step).format({ mantissa: countDecimals(props.step) }));
if ((!hasProperty(props, 'min') || newValue >= props.min) && (!hasProperty(props, 'max') || (newValue <= props.max))) {
const { showError, errorMsg } = validNumber(newValue, props.min, props.max);
context.updateState({
showError,
errorMsg,
value: toCurrency(newValue, countDecimals(props.step))
}, () => {
if (typeof props.onChange === 'function') {
if (greaterThanMin(newValue) && lessThanMax(newValue)) {
const updateError = displayErrorMessage(!is.empty(stringValue) ? newValue : '');
context.updateState({ value: toCurrency(newValue, countDecimals(props.step)), ...updateError }, () => {
if (is.fn(props.onChange)) {
props.onChange(newValue, props.id, type, key);
}
});
}
} else if (key === 'ArrowUp') {
newValue = Number(numbro(numberValue).add(props.step).format({ mantissa: countDecimals(props.step) }));
if ((!hasProperty(props, 'min') || newValue >= props.min) && (!hasProperty(props, 'max') || (newValue <= props.max))) {
const { showError, errorMsg } = validNumber(newValue, props.min, props.max);
context.updateState({
showError,
errorMsg,
value: toCurrency(newValue, countDecimals(props.step))
}, () => {
if (typeof props.onChange === 'function') {
if (greaterThanMin(newValue) && lessThanMax(newValue)) {
const updateError = displayErrorMessage(!is.empty(stringValue) ? newValue : '');
context.updateState({ value: toCurrency(newValue, countDecimals(props.step)), ...updateError }, () => {
if (is.fn(props.onChange)) {
props.onChange(newValue, props.id, type, key);
}
});
}
}
}
};

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

@smurrayatwork smurrayatwork Mar 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is doing the same thing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because stringValue can't be number 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

avrilpearl marked this conversation as resolved.
Show resolved Hide resolved
if (isNotNumber) {
inputEl.setAttribute('placeholder', props.placeholder);
}
const stringValue = inputEl.value;
const numberValue = Number(numbro.unformat(stringValue));
let newValue = isNotNumber ? '' : Number(numbro.unformat(stringValue));
if (props.required && is.empty(stringValue)) {
errorMsg = 'Please enter a value.';
context.updateState({ showError: true, errorMsg });
} else if (!is.empty(stringValue)) {
let newValue = numberValue;
if (hasProperty(props, 'max') && newValue > props.max) {
if (!hasNumberProperty(props, 'max') || newValue > props.max) {
avrilpearl marked this conversation as resolved.
Show resolved Hide resolved
newValue = props.max;
}
if (hasProperty(props, 'min') && newValue < props.min) {
if (!hasNumberProperty(props, 'min') || newValue < props.min) {
avrilpearl marked this conversation as resolved.
Show resolved Hide resolved
newValue = props.min;
}
const { showError, errorMsg } = validNumber(newValue, props.min, props.max);
context.updateState({ showError, errorMsg, value: toCurrency(newValue, countDecimals(props.step)) }, () => {
// invokes custom function if passed in the component
const updateError = displayErrorMessage(!is.empty(stringValue) ? newValue : '');
context.updateState({ value: toCurrency(newValue, countDecimals(props.step)), ...updateError }, () => {
if (is.fn(props.onBlur)) {
// context.value won't be immediately changed, so pass new value over.
props.onBlur(numberValue);
props.onBlur(newValue);
}
});
}
};

const handleFocus = () => {
const inputEl = ref.current;
if (is.empty(inputEl.value)) {
inputEl.removeAttribute('placeholder');
}
};

const inputAttr = {
className: inputClasses,
name: props.name,
Expand All @@ -207,6 +189,7 @@ const Currency = (props) => {
value: context.getValue(),
disabled: props.disabled
};

return(
<div className="ma__input-currency">
<input {...inputAttr} />
Expand Down
73 changes: 32 additions & 41 deletions react/src/components/atoms/forms/InputNumber/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,45 +52,34 @@ const NumberInput = (props) => {
errorMsg: ''
};
};
const hasProperty = (obj, property) => Object.prototype.hasOwnProperty.call(obj, property) && !is.nil(obj[property]);

const hasNumberProperty = (obj, property) => Object.prototype.hasOwnProperty.call(obj, property) && is.number(obj[property]);

const handleOnBlur = (e) => {
e.persist();
const inputEl = ref.current;
let newValue = Number(inputEl.value);
if ((hasProperty(props, 'max') && newValue > props.max) || (hasProperty(props, 'min') && newValue < props.min)) {
if (hasProperty(props, 'max') && newValue > props.max) {
newValue = props.max;
}
if (hasProperty(props, 'min') && newValue < props.min) {
newValue = props.min;
}
let newValue = inputEl.value ? Number(inputEl.value) : inputEl.value;
avrilpearl marked this conversation as resolved.
Show resolved Hide resolved
if (hasNumberProperty(props, 'max') && newValue > props.max) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

This comment was marked as outdated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

This comment was marked as outdated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

newValue = props.min;
}
if (is.number(newValue)) {
newValue = newValue.toFixed(countDecimals(props.step));
const updateError = displayErrorMessage(newValue);
context.updateState({ value: inputEl.value, ...updateError }, () => {
context.updateState({ value: newValue, ...updateError }, () => {
if (is.fn(props.onBlur)) {
props.onBlur(e, inputEl.value);
props.onBlur(e, newValue);
}
});
}
};

const handleChange = (e) => {
const inputEl = ref.current;
e.persist();
let newValue;
if (is.empty(inputEl.value)) {
newValue = inputEl.value;
} else {
newValue = Number(inputEl.value);
if (is.number(newValue)) {
newValue = Number(numbro(inputEl.value)
.format({ mantissa: countDecimals(props.step) }));
}
}
const inputEl = ref.current;
const newValue = inputEl.value ? Number(inputEl.value) : inputEl.value;
const updateError = displayErrorMessage(newValue);
context.updateState({ value: newValue, ...updateError }, () => {
if (is.fn(props.onChange)) {
Expand All @@ -100,29 +89,31 @@ const NumberInput = (props) => {
};

const handleAdjust = (e) => {
e.persist();
let direction;
if (e.currentTarget === upRef.current) {
direction = 'up';
} else {
direction = 'down';
}
const inputEl = ref.current;
if (direction === 'up' && (!hasProperty(props, 'max') || inputEl.value < props.max)) {
if (is.empty(inputEl.value)) {
inputEl.value = 1;
} else {
inputEl.value = Number(numbro(inputEl.value)
.add(props.step).value());
}
handleChange(e);
} else if (direction === 'down' && (!hasProperty(props, 'min') || inputEl.value > props.min)) {
if (is.empty(inputEl.value)) {
inputEl.value = -1;
} else {
inputEl.value = Number(numbro(inputEl.value)
.subtract(props.step).value());
}
handleChange(e);
let newValue = inputEl.value ? Number(inputEl.value) : inputEl.value;
if (direction === 'up' && (!hasNumberProperty(props, 'max') || newValue < props.max)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

This comment was marked as outdated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

newValue = newValue ? (newValue + props.step).toFixed(countDecimals(props.step)) : props.step;
avrilpearl marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

This comment was marked as outdated.

This comment was marked as outdated.

Copy link
Contributor

@avrilpearl avrilpearl Mar 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const updateError = displayErrorMessage(newValue);
context.updateState({ value: newValue, ...updateError }, () => {
if (is.fn(props.onChange)) {
props.onChange(e, newValue, props.id);
}
});
} else if (direction === 'down' && (!hasNumberProperty(props, 'min') || newValue > props.min)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

newValue = newValue ? (newValue + props.step * -1).toFixed(countDecimals(props.step)) : (props.step * -1);
avrilpearl marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

This comment was marked as outdated.

Copy link
Contributor

@avrilpearl avrilpearl Mar 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const updateError = displayErrorMessage(newValue);
context.updateState({ value: newValue, ...updateError }, () => {
if (is.fn(props.onChange)) {
props.onChange(e, newValue, props.id);
}
});
}
};

Expand Down