Localize decimal separator #9800

eVoloshchak · 2022-07-08T09:45:23Z

Details

This change allows user to use comma (,) instead of a dot (.) as a decimal separator on WorkspaceReimburseView and IOUAmountPage when using Spanish language.

Fixed Issues

$ #9497

Tests

Have English language enabled
Click FAB > Request money
Verify that there is . button at the bottom left corner of the onscreen keyboard
Verify that it's possible to enter a decimal number with two digits after the dot
Open Settings > Workspace > Reimburse expenses
Verify that it's possible to enter a decimal number with three digits after the dot into Rate input
Change language to Spanish
Click FAB > Request money
Verify that there is , button at the bottom left corner of the onscreen keyboard
Verify that it's possible to enter a decimal number with two digits after the comma
Open Settings > Workspace > Reimburse expenses
Verify that it's possible to enter a decimal number with three digits after the comma into Rate input

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Have English language enabled
Click FAB > Request money
Verify that there is . button at the bottom left corner of the onscreen keyboard
Verify that it's possible to enter a decimal number with two digits after the dot
Open Settings > Workspace > Reimburse expenses
Verify that it's possible to enter a decimal number with three digits after the dot into Rate input
Change language to Spanish
Click FAB > Request money
Verify that there is , button at the bottom left corner of the onscreen keyboard
Verify that it's possible to enter a decimal number with two digits after the comma
Open Settings > Workspace > Reimburse expenses
Verify that it's possible to enter a decimal number with three digits after the comma into Rate input

Screenshots

Web

video_2022-07-13_14-33-58.mp4

cinnamon-20220707-5.mp4

Mobile Web

22-07-07-23-08-01.mp4

iOS

cinnamon-20220708-9.MP4

Android

22-07-08-12-13-35.mp4

Santhosh-Sellavel · 2022-07-11T17:23:33Z

@eVoloshchak Can you include desktop recording too & also please check all the checkboxes in the author checklist.

Santhosh-Sellavel · 2022-07-11T17:29:13Z

@eVoloshchak Check your web video from 00 to 06 secs. I see value getting lost temporarily. You are entering 233.32, it appears 33.32 for fraction of seconds.

sketchydroide

sorry I aproved while only seeing the code, but @Santhosh-Sellavel is right the web video looks off

eVoloshchak · 2022-07-13T11:36:36Z

@eVoloshchak Check your web video from 00 to 06 secs. I see value getting lost temporarily. You are entering 233.32, it appears 33.32 for fraction of seconds.

I think that my screen recorder is recording at low fps, what i see on the screen and on the video are not the same thing. I updated the web section with a screen capture shot on a phone camera

video_2022-07-13_14-33-58.mp4

eVoloshchak · 2022-07-13T11:39:09Z

@eVoloshchak Can you include desktop recording too & also please check all the checkboxes in the author checklist.

I can't run in on desktop unfortunately, due to #8888 it won't run using recent node versions, and i can't install older node since I use macInCloud

Santhosh-Sellavel · 2022-07-13T17:54:50Z

@eVoloshchak Check your web video from 00 to 06 secs. I see value getting lost temporarily. You are entering 233.32, it appears 33.32 for fraction of seconds.

I think that my screen recorder is recording at low fps, what I see on the screen and on the video are not the same thing. I updated the web section with a screen capture shot on a phone camera

video_2022-07-13_14-33-58.mp4

Please update the screen record so that's a more reliable option, to ensure everything. I believe you can do that on macincloud also.

eVoloshchak · 2022-07-14T13:39:01Z

Please update the screen record so that's a more reliable option, to ensure everything. I believe you can do that on macincloud also.

Sure! This is a screen recording from Windows machine, it seems to have more fps.

1.New.Expensify.-.Google.Chrome.2022-07-14.15-44-32.mp4

Santhosh-Sellavel · 2022-07-15T16:49:09Z

@eVoloshchak can you repeat the same steps in staging and share the recording. With that, we can rule out whether the behavior is caused by this PR or not thanks!

Please update the screen record so that's a more reliable option, to ensure everything. I believe you can do that on macincloud also.

Sure! This is a screen recording from Windows machine, it seems to have more fps.

1.New.Expensify.-.Google.Chrome.2022-07-14.15-44-32.mp4

eVoloshchak · 2022-07-16T15:20:35Z

@eVoloshchak can you repeat the same steps in staging and share the recording. With that, we can rule out whether the behavior is caused by this PR or not thanks!

Of course

New.Expensify.-.Google.Chrome.2022-07-16.18-17-54.mp4

Santhosh-Sellavel · 2022-07-18T17:59:57Z

PR Reviewer Checklist

I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
I verified there are no console errors (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained “why” the code was doing something instead of only explaining “what” the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named “index.js”. All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- Any functional components have the displayName property
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn’t already exist
- The style can’t be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.

Santhosh-Sellavel

Looks good and tests well @sketchydroide.

sketchydroide · 2022-07-19T13:35:19Z

@eVoloshchak great job getting your first Expensify/App pull request over the finish line! I'll merge it assuming all the tests pass.

I know there's a lot of information in our contributing guidelines, so some points to take note of:

Once your PR is merged, you can be hired for another issue. Once you've completed a few issues, we may start hiring you for more than one issue at a time.

Once your PR is deployed to our staging servers, it will undergo quality assurance (QA) testing. If we find that this doesn't work as expected or causes a regression, you'll be responsible for fixing it. Typically we would revert this PR and give you another chance to create a similar PR without causing a regression. (I don't imagine this will happen with this PR, but it's something to be aware of)

Once your PR is deployed to production, we start a 7-day timer. After it has been on production for 7 days without causing any regressions, then we pay out the Upwork job.

So it might take a while before you're paid for your work, but we typically post multiple new jobs every day, so there's plenty of opportunity. I hope you've had a positive experience contributing to this repo!

OSBotify · 2022-07-19T13:41:32Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

eVoloshchak · 2022-07-19T14:35:43Z

@eVoloshchak great job getting your first Expensify/App pull request over the finish line!

Not my first Expensify/App pull request, but still thanks 😄

parasharrajat

I was checking some other PR and saw these changes to IOUAmountPage which I feel like very off from the planned design of this page.

I tested the IOUAmountPage before this PR and it was accepting , instead of . for Spanish locale.

Why do we have to change IOUAmountPage here? First of all, How is this related to this issue?

cc: @Santhosh-Sellavel

parasharrajat · 2022-07-20T14:19:37Z

src/components/BigNumberPad.js

@@ -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))}


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

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

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.

@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

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?

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

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?

Keyboard should show , for spanish.

Textinput or amount input should show , for spanish.

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

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?

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

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.

parasharrajat · 2022-07-20T14:29:26Z

src/pages/workspace/reimburse/WorkspaceReimburseView.js

    }

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


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(',');

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.

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

Easy to miss, thanks @parasharrajat!

Need to update this @eVoloshchak!

cc: @sketchydroide

Need to update this @eVoloshchak!

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

@sketchydroide are we okay with it?

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

OSBotify · 2022-07-25T18:04:39Z

🚀 Deployed to staging by @sketchydroide in version: 1.1.86-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-08-01T22:12:27Z

🚀 Deployed to production by @yuwenmemon in version: 1.1.86-5 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

mountiny · 2022-08-02T08:41:22Z

@eVoloshchak and @parasharrajat the linked issue has been incorrectly linked:

$ https://github.com/Expensify/App/issues/9497 the dollar sign was missing. Please, pay attention to this for next PRs. Thank you 🙇

parasharrajat · 2022-08-02T09:10:12Z

Santhosh-Sellavel was C+ here. But thanks for the note.

mountiny · 2022-08-02T09:25:23Z

Sorry Rajat, saw you amongst the reviewers. @Santhosh-Sellavel for the note above 🙇

Santhosh-Sellavel · 2022-08-02T19:30:06Z

🤦 Oops

@mountiny Thanks for the note!

puneetlath · 2022-11-10T20:26:41Z

Heads up @parasharrajat @Santhosh-Sellavel @sketchydroide! Just FYI: this PR caused a bug here: #11352

parasharrajat · 2022-11-10T20:31:27Z

I already asked to revert it #9800 (comment) because I didn't agree with the changes. we had a revert PR #10129.

There must be follow up PR. cc: @Santhosh-Sellavel as C+.

Santhosh-Sellavel · 2022-11-11T00:06:19Z

@puneetlath This PR was reverted not sure how it caused a regression!

puneetlath · 2022-11-15T18:44:19Z

Good point fellas. My bad lol. Will look further into it!

eVoloshchak added 5 commits July 7, 2022 22:03

Output localized decimal separator from keyboard

59f4c9d

Use localized separator on reimbursment screen

08b075d

Fix linter error

30b40bd

Use localized separator on IOUAmountPage

9d80857

Cleanup

bcf54e6

eVoloshchak requested a review from a team as a code owner July 8, 2022 09:45

melvin-bot bot requested review from Santhosh-Sellavel and sketchydroide and removed request for a team July 8, 2022 09:45

sketchydroide approved these changes Jul 13, 2022

View reviewed changes

sketchydroide suggested changes Jul 13, 2022

View reviewed changes

Santhosh-Sellavel approved these changes Jul 18, 2022

View reviewed changes

sketchydroide approved these changes Jul 19, 2022

View reviewed changes

sketchydroide merged commit 7541b77 into Expensify:main Jul 19, 2022

parasharrajat reviewed Jul 20, 2022

View reviewed changes

parasharrajat mentioned this pull request Jul 20, 2022

[HOLD for payment 2022-09-26] [$250] User can't use , (comma) In spanish for decimals during IOU - reported by @thesahindia #9497

Closed

OSBotify mentioned this pull request Jul 25, 2022

Deploy Checklist: New Expensify 2022-07-25 #10077

Closed

92 tasks

eVoloshchak mentioned this pull request Jul 25, 2022

[HOLD for payment 2022-09-07] [$2000] IOU - User is unable to modify the IOU amount - Reported by: @Santhosh-Sellavel #6154

Closed

kavimuru mentioned this pull request Jul 26, 2022

Workspace - Spanish - Decimal separator comma is missing on the keyboard and dot is non functional on keyboard #10108

Closed

sketchydroide mentioned this pull request Jul 27, 2022

Revert "Localize decimal separator" #10129

Merged

thesahindia mentioned this pull request Nov 9, 2022

[HOLD for payment 2022-11-16] [$500] Android/iOS - Workspace - Numbers shaking while updating rate in reimburse expenses @gadhiyamanan #11352

Closed

Localize decimal separator #9800

Localize decimal separator #9800

Conversation

eVoloshchak commented Jul 8, 2022 • edited by mountiny Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

iOS

Android

Santhosh-Sellavel commented Jul 11, 2022

Santhosh-Sellavel commented Jul 11, 2022

sketchydroide left a comment • edited Loading

Choose a reason for hiding this comment

eVoloshchak commented Jul 13, 2022

eVoloshchak commented Jul 13, 2022

Santhosh-Sellavel commented Jul 13, 2022

eVoloshchak commented Jul 14, 2022

Santhosh-Sellavel commented Jul 15, 2022 • edited Loading

eVoloshchak commented Jul 16, 2022

Santhosh-Sellavel commented Jul 18, 2022 • edited Loading

PR Reviewer Checklist

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

sketchydroide commented Jul 19, 2022

OSBotify commented Jul 19, 2022

eVoloshchak commented Jul 19, 2022

parasharrajat left a comment • 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

parasharrajat Jul 20, 2022 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Jul 25, 2022

OSBotify commented Aug 1, 2022

mountiny commented Aug 2, 2022

parasharrajat commented Aug 2, 2022 • edited Loading

mountiny commented Aug 2, 2022

Santhosh-Sellavel commented Aug 2, 2022

puneetlath commented Nov 10, 2022

parasharrajat commented Nov 10, 2022 • edited Loading

Santhosh-Sellavel commented Nov 11, 2022

puneetlath commented Nov 15, 2022

eVoloshchak commented Jul 8, 2022 •

edited by mountiny

Loading

sketchydroide left a comment •

edited

Loading

Santhosh-Sellavel commented Jul 15, 2022 •

edited

Loading

Santhosh-Sellavel commented Jul 18, 2022 •

edited

Loading

parasharrajat left a comment •

edited

Loading

parasharrajat Jul 20, 2022 •

edited

Loading

parasharrajat commented Aug 2, 2022 •

edited

Loading

parasharrajat commented Nov 10, 2022 •

edited

Loading