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

Localize decimal separator #9800

Merged
merged 5 commits into from
Jul 19, 2022
Merged

Localize decimal separator #9800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
59f4c9d
Output localized decimal separator from keyboard
eVoloshchak Jul 7, 2022
08b075d
Use localized separator on reimbursment screen
eVoloshchak Jul 7, 2022
30b40bd
Fix linter error
eVoloshchak Jul 7, 2022
9d80857
Use localized separator on IOUAmountPage
eVoloshchak Jul 7, 2022
bcf54e6
Cleanup
eVoloshchak Jul 8, 2022
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
1 change: 0 additions & 1 deletion src/CONST.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -680,7 +680,6 @@ const CONST = {
CARD_SECURITY_CODE: /^[0-9]{3,4}$/,
CARD_EXPIRATION_DATE: /^(0[1-9]|1[0-2])([^0-9])?([0-9]{4}|([0-9]{2}))$/,
PAYPAL_ME_USERNAME: /^[a-zA-Z0-9]+$/,
RATE_VALUE: /^\d{1,8}(\.\d*)?$/,

// Adapted from: https://gist.github.com/dperini/729294
// eslint-disable-next-line max-len
Expand Down
2 changes: 1 addition & 1 deletion src/components/BigNumberPad.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ class BigNumberPad extends React.Component {
style={[styles.flex1, marginLeft]}
text={column === '<' ? column : this.props.toLocaleDigit(column)}
onLongPress={() => this.handleLongPress(column)}
onPress={() => this.props.numberPressed(column)}
onPress={() => this.props.numberPressed(column === '<' ? column : this.props.toLocaleDigit(column))}
Copy link
Member

Choose a reason for hiding this comment

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

How is this change related to this PR? cc: @Santhosh-Sellavel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How is this change related to this PR? cc: @Santhosh-Sellavel

Previously BigNumberPad was displaying , while still passing . to this.props.numberPressed on Spanish, now it's passing the sybmol that is displayed on the key

Copy link
Member

Choose a reason for hiding this comment

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

That is how it is supposed to work Internally the values will always be in English for eg.
22,23 is really 22.23 which will be saved to the backend. So it was working correctly.

Manipulating this value here is not a good approach. These changes break the whole concept.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@parasharrajat

We are not saving it as 22,23 or 22.33. We are rounding it off after multiplying by a hundred, so it's saved as 2233. I'm not sure about what we broke here?

cc: @sketchydroide

Copy link
Member

@parasharrajat parasharrajat Jul 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

by the backend, I mean internally not DB. Why do we have to change this piece of code? It does not look related to the issue. Can anyone please help me understand this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do we have to change this piece of code? It does not look related to the issue. Can anyone please help me understand this?

This keyboard is used only on IOUAmountPage, so it seemed logical to make it return the displayed symbol.
Alternatively we can move this logic to IOUAmountPage, change this line to

const amount = `${prevState.amount}${this.props.toLocaleDigit(key)}`;

Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this change? Commit before the merge commit shows the correct behavior of showing , on UI and Allowing the user to type , for Spanish and . for English.

This keyboard is used only on IOUAmountPage, so it seemed logical to make it return the displayed symbol.

How?

  1. Keyboard should show , for spanish.
  2. Textinput or amount input should show , for spanish.

Both of these were already happening so why is change needed?

Copy link
Member

Choose a reason for hiding this comment

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

Do you think there was any bug on IOUAmountPage that you solved here? What was that? Issue talks about the problem with the rate field on the reimbursement form. Can we fix the rate field issue without touching IOUAmountPage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think there was any bug on IOUAmountPage that you solved here? What was that? Issue talks about the problem with the rate field on the reimbursement form.

But the screenshot in the issue description features Request money screen. I've asked about this a couple of times, there must have been a miscommunication

OK, i think i got confused, since the issue description is talking about the Rate input on Reimburce Expenses screen, while screenshot contains IOU Amount Page with an amount input
I'm still not sure which one of the two screens needs to be fixed

Can we fix the rate field issue without touching IOUAmountPage?

Yes we can
I've checked new.expensify.com and it does indeed support comma for Spanish language on Request Money screen.
I'm guessing we need o revert this PR and clarify what the requirements for this issue are, it it's just for the rate field

Copy link
Member

Choose a reason for hiding this comment

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

It seems yes issue details are incorrect. Unfortunately, it was not captured during the proposal review. As a suggestion, please try to get a full consensus before you start if you feel something is off. You can also suggest corrections to the requirements.

Thanks for understanding. I will open this discussion on the issue so that this PR can be reverted and you can start new.

onPressIn={ControlSelection.block}
onPressOut={() => {
clearInterval(this.state.timer);
Expand Down
57 changes: 9 additions & 48 deletions src/pages/iou/steps/IOUAmountPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {
import PropTypes from 'prop-types';
import {withOnyx} from 'react-native-onyx';
import lodashGet from 'lodash/get';
import _ from 'underscore';
import ONYXKEYS from '../../../ONYXKEYS';
import styles from '../../../styles/styles';
import BigNumberPad from '../../../components/BigNumberPad';
Expand Down Expand Up @@ -59,12 +58,11 @@ class IOUAmountPage extends React.Component {

this.updateAmountNumberPad = this.updateAmountNumberPad.bind(this);
this.updateAmount = this.updateAmount.bind(this);
this.stripCommaFromAmount = this.stripCommaFromAmount.bind(this);
this.focusTextInput = this.focusTextInput.bind(this);
this.navigateToCurrencySelectionPage = this.navigateToCurrencySelectionPage.bind(this);

this.state = {
amount: props.selectedAmount,
amount: props.selectedAmount.replace('.', this.props.fromLocaleDigit('.')),
};
}

Expand Down Expand Up @@ -119,20 +117,11 @@ class IOUAmountPage extends React.Component {
* @returns {Boolean}
*/
validateAmount(amount) {
const decimalNumberRegex = new RegExp(/^\d+(,\d+)*(\.\d{0,2})?$/, 'i');
const decimalSeparator = this.props.fromLocaleDigit('.');
const decimalNumberRegex = RegExp(String.raw`^\d+([${decimalSeparator}]\d{0,2})?$`, 'i');
return amount === '' || (decimalNumberRegex.test(amount) && this.calculateAmountLength(amount) <= CONST.IOU.AMOUNT_MAX_LENGTH);
}

/**
* Strip comma from the amount
*
* @param {String} amount
* @returns {String}
*/
stripCommaFromAmount(amount) {
return amount.replace(/,/g, '');
}

/**
* Update amount with number or Backspace pressed for BigNumberPad.
* Validate new amount with decimal number regex up to 6 digits and 2 decimal digit to enable Next button
Expand All @@ -152,44 +141,18 @@ class IOUAmountPage extends React.Component {

this.setState((prevState) => {
const amount = `${prevState.amount}${key}`;
return this.validateAmount(amount) ? {amount: this.stripCommaFromAmount(amount)} : prevState;
return this.validateAmount(amount) ? {amount} : prevState;
});
}

/**
* Update amount on amount change
* Validate new amount with decimal number regex up to 6 digits and 2 decimal digit
*
* @param {String} text - Changed text from user input
*/
updateAmount(text) {
this.setState((prevState) => {
const amount = this.replaceAllDigits(text, this.props.fromLocaleDigit);
return this.validateAmount(amount)
? {amount: this.stripCommaFromAmount(amount)}
: prevState;
});
}

/**
* Replaces each character by calling `convertFn`. If `convertFn` throws an error, then
* the original character will be preserved.
*
* @param {String} text
* @param {Function} convertFn - `this.props.fromLocaleDigit` or `this.props.toLocaleDigit`
* @returns {String}
* @param {String} amount - Changed amount from user input
*/
replaceAllDigits(text, convertFn) {
return _.chain([...text])
.map((char) => {
try {
return convertFn(char);
} catch {
return char;
}
})
.join('')
.value();
updateAmount(amount) {
this.setState(prevState => (this.validateAmount(amount) ? {amount} : prevState));
}

navigateToCurrencySelectionPage() {
Expand All @@ -203,8 +166,6 @@ class IOUAmountPage extends React.Component {
}

render() {
const formattedAmount = this.replaceAllDigits(this.state.amount, this.props.toLocaleDigit);

return (
<>
<View style={[
Expand All @@ -216,7 +177,7 @@ class IOUAmountPage extends React.Component {
]}
>
<TextInputWithCurrencySymbol
formattedAmount={formattedAmount}
formattedAmount={this.state.amount}
onChangeAmount={this.updateAmount}
onCurrencyButtonPress={this.navigateToCurrencySelectionPage}
placeholder={this.props.numberFormat(0)}
Expand All @@ -236,7 +197,7 @@ class IOUAmountPage extends React.Component {
<Button
success
style={[styles.w100, styles.mt5]}
onPress={() => this.props.onStepComplete(this.state.amount)}
onPress={() => this.props.onStepComplete(this.state.amount.replace(this.props.fromLocaleDigit('.'), '.'))}
pressOnEnter
isDisabled={!this.state.amount.length || parseFloat(this.state.amount) < 0.01}
text={this.props.translate('common.next')}
Expand Down
17 changes: 5 additions & 12 deletions src/pages/workspace/reimburse/WorkspaceReimburseView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,16 +77,13 @@ class WorkspaceReimburseView extends React.Component {
}

getRateDisplayValue(value) {
const numValue = parseFloat(value);
if (Number.isNaN(numValue)) {
return '';
}

return numValue.toFixed(3);
return value.toString().replace('.', this.props.fromLocaleDigit('.'));
}

setRate(value) {
const isInvalidRateValue = value !== '' && !CONST.REGEX.RATE_VALUE.test(value);
const decimalSeparator = this.props.fromLocaleDigit('.');
Copy link
Member

Choose a reason for hiding this comment

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

use of fromLocaleDigit is wrong here as per your changes. Why?

fromLocaleDigit accepts a localized char, . is not decimal in spanish so it should this.props.fromLocaleDigit(',');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

use of fromLocaleDigit is wrong here as per your changes. Why?

Thanks, that is a good catch. They return the same value when passing . and I missed it and used the wrong function on IOUAmountPage, while using the right one on BigNumberPad. I'll open a PR with fix for this after the discussion around the BigNumberPad.

Copy link
Collaborator

Choose a reason for hiding this comment

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

. is the grouping separator in Spanish, i.e , in English.

Easy to miss, thanks @parasharrajat!

Need to update this @eVoloshchak!

cc: @sketchydroide

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to update this @eVoloshchak!

I can fix this in #10028, would that be ok?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sketchydroide are we okay with it?

Copy link
Member

Choose a reason for hiding this comment

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

Please hold on to it. We are first trying to decide why are these changes needed at all.

const rateValueRegex = RegExp(String.raw`^\d{1,8}([${decimalSeparator}]\d{0,3})?$`, 'i');
const isInvalidRateValue = value !== '' && !rateValueRegex.test(value);

this.setState(prevState => ({
rateValue: !isInvalidRateValue ? value : prevState.rateValue,
Expand Down Expand Up @@ -115,16 +112,12 @@ class WorkspaceReimburseView extends React.Component {
}

updateRateValue(value) {
const numValue = parseFloat(value);
const numValue = parseFloat(value.replace(this.props.fromLocaleDigit('.'), '.'));

if (_.isNaN(numValue)) {
return;
}

this.setState({
rateValue: numValue.toFixed(3),
});

Policy.setCustomUnitRate(this.props.policyID, this.state.unitID, {
customUnitRateID: this.state.rateID,
name: this.state.rateName,
Expand Down