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.

Sign up for GitHub

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

react/fix inputNumber handleAdjust logic #518

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
c94d3e2
fix inputNumber handleAdjust logic
clairesunstudio Mar 26, 2019
ac6a0fc
fix input currency min/max logic
clairesunstudio Mar 26, 2019
19fc2ed
refactor
clairesunstudio Mar 26, 2019
ec4491f
add changelog
clairesunstudio Mar 26, 2019
6fc1e03
Update react/src/components/atoms/forms/InputNumber/index.js
smurrayatwork Mar 26, 2019
c6d244f
simplify input number handle change logic
avrilpearl Mar 28, 2019
560b2d7
simply handle blur and handle adjust on input number
avrilpearl Mar 28, 2019
5b931aa
wip
avrilpearl Mar 28, 2019
d93df58
update input currency
avrilpearl Mar 29, 2019
1cba9f9
Update changelogs/DP-13167.md
clairesunstudio Mar 29, 2019
34d8a1d
Update react/src/components/atoms/forms/InputCurrency/index.js
smurrayatwork Mar 29, 2019
6660ae8
Update react/src/components/atoms/forms/InputCurrency/index.js
clairesunstudio Mar 29, 2019
f57dadc
Update react/src/components/atoms/forms/InputCurrency/index.js
clairesunstudio Mar 29, 2019
82edafc
Update react/src/components/atoms/forms/InputCurrency/index.js
clairesunstudio Mar 29, 2019
5a64039
cast to fixed back to number
avrilpearl Mar 29, 2019
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
View file
Open in desktop
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. #518
clairesunstudio marked this conversation as resolved.
Show resolved Hide resolved
14 changes: 8 additions & 6 deletions react/src/components/atoms/forms/InputCurrency/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ 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 handleChange = (e) => {
const { type } = e;
const stringValue = ref.current.value;
Expand Down Expand Up @@ -98,7 +100,7 @@ const Currency = (props) => {
} 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))) {
if (greaterThanMin(newValue) && lessThanMax(newValue)) {
const { showError, errorMsg } = validNumber(newValue, props.min, props.max);
context.updateState({
showError,
Expand Down Expand Up @@ -126,7 +128,7 @@ const Currency = (props) => {
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))) {
if (greaterThanMin(newValue) && lessThanMax(newValue)) {
const { showError, errorMsg } = validNumber(newValue, props.min, props.max);
context.updateState({
showError,
Expand All @@ -140,7 +142,7 @@ const Currency = (props) => {
}
} 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))) {
if (greaterThanMin(newValue) && lessThanMax(newValue)) {
const { showError, errorMsg } = validNumber(newValue, props.min, props.max);
context.updateState({
showError,
Expand All @@ -167,10 +169,10 @@ const Currency = (props) => {
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);
Expand Down
73 changes: 32 additions & 41 deletions react/src/components/atoms/forms/InputNumber/index.js
View file
Open in desktop
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.

Sign in to view

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.

Sign in to view

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.

Sign in to view

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.

Sign in to view

This comment was marked as outdated.

Sign in to view

Copy link
Contributor

@avrilpearl avrilpearl Mar 29, 2019 •
edited
Loading

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.

Sign in to view

Copy link
Contributor

@avrilpearl avrilpearl Mar 29, 2019 •
edited
Loading

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