feat(amount-input-screen): allow for pasting values in amount input s… #2854

MaxwellDG · 2023-11-29T17:52:03Z

Addresses issue #2497

I've added the ability to paste into the amount screen.
This accounts for:

validating that it's only numerical and/or decimals
not opening the keyboard
maintaining that there can varying amounts of integers after the decimal (depending on the currency)
maintains auto-formatting with commas for ease of viewing
combining the stateful input of pastes with numpad inputs

PS While working on this I noticed a large problem. There are dozens of currencies that use commas instead of decimals to differentiate between the 'dollar' and 'cent' (or whatever they call them) values. The library we're using for countries accounts for this but our numpad and input does not. This means anyone trying to input partial values is blocked. If you'd like to see for yourself, try Albanian money. I've opened an issue for this #2853

…creen

nicolasburtey · 2023-12-01T20:21:12Z

There are dozens of currencies that use commas instead of decimals to differentiate between the 'dollar' and 'cent' (or whatever they call them) values.

this has less to do with currency but more with the language / country settings actually.

ie: in France, we would show
100€
100$

while in the US we would have:
€100
$100

In Javascript, this would be a way on how to properly show the currency amount: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat/NumberFormat

app/components/amount-input-screen/amount-input-screen-ui.tsx

nicolasburtey · 2023-12-01T21:21:10Z

app/components/amount-input-screen/number-pad-reducer.ts

+      if (action.payload.key.length > 1) {
+        // here the whole numberPadNumber will be formatted and replaced
+        // unlike the several sections below which append to the value(s)
+        const num = Number(action.payload.key)
+        // format to float if number has decimal - otherwise integer
+        const formatted: string =
+          num % 1 === 0 ? num.toString() : num.toFixed(numberOfDecimalsAllowed)
+        const splitByDecimal = formatted.split(".")
+
+        return {
+          ...state,
+          numberPadNumber: {
+            majorAmount: splitByDecimal[0],
+            hasDecimal: splitByDecimal.length > 1,
+            minorAmount: splitByDecimal[1] ?? "",
+          },
+        }
+      }


while I think the code is working, I don't think this is the cleaner way to manage the paste of a code

we use a reducer to have a clean boundary between different action, and it would be better to have an action dedicated to the use case when the user paste a code. effectively what this would looks like is that there would be a third action, ie case NumberPadReducerActionType.PasteAmount that would trigger this piece of code.

this would also avoid the casting here https://github.com/GaloyMoney/galoy-mobile/pull/2854/files#diff-f90138f20708f2a12f53a72c46c65505a8dd601948f85ee3f6ca3f22a6c8c6c4R67

this casting is misleading because we force a large value to be a Key which by definition should be a single digit

Yea i get that. Ill be back online in a couple days and Ill make a new reducer action. Makes sense following that pattern more

@nicolasburtey The type for 'Keys' I added felt a bit weird, but I believe its the best option. You'll notice starting from amount-input-screen-ui.tsx line 69 I go from string -> Number -> string -> Number -> string again. If you think that's weird let me know and I can explain the rationale.

nicolasburtey · 2023-12-01T21:22:22Z

The Logic on the PR looks good but I think we need an refactoring iteration so that the code doesn't casting and has better function boundary

nicolasburtey · 2023-12-06T19:57:47Z

app/components/amount-input-screen/amount-input-screen-ui.tsx

+              onChangeText={(e) => {
+                // remove commas for ease of calculation later on
+                const val = e.replaceAll(",", "")
+                // TODO adjust for currencies that use commas instead of decimals
+
+                // test for string input that can be either numerical or float
+                if (/^\d*\.?\d*$/.test(val)) {
+                  const num = Number(val)
+                  onPaste(`${num}`)
+                }
+              }}


is this code executed both for the case when we add a key or when we paste a string?

I double-checked, and no - it's only when the user changes the input field directly themselves.

nicolasburtey · 2023-12-06T19:59:16Z

app/components/amount-input-screen/number-pad-reducer.ts

@@ -54,6 +61,8 @@ export const Key = {

 export type Key = (typeof Key)[keyof typeof Key]

+export type Keys = `${number | ""}${"." | ""}${number | ""}`


assuming Keys is what the user can paste, I don't think it make sense really a type. because ultimately the user could paste anything: a string, a number. I think we will want to validate / trim the input to make sure that if the user paste something incorrect, this is correctly handled.

said another way, I don't think we need a Keys type at all, and we can accept a string instead that should be tentatively converted to a number.

Yea i agree the Keys type seemed unnecessary. Removing that would streamline the code here too by removing unnecessary type conversions. Ill adjust the whole process to accept a number.

The safety and validation is already there too. Note the regex matching on line 68 in amount-input-screen-ui.tsx.

oh and i'll throw in that trim() too

nicolasburtey · 2023-12-06T20:01:23Z

The PR looks much better overall, but I think we need a proper validation on what the user is pasting, to properly manage the case when the input is not convertible into a number.

MaxwellDG · 2023-12-06T21:19:11Z

The PR looks much better overall, but I think we need a proper validation on what the user is pasting, to properly manage the case when the input is not convertible into a number.

You don't think the regex string matching is enough? Currently if the value can't be formatted into a number then it is a no-op and doesn't allow it to be pasted.

nicolasburtey · 2023-12-08T14:24:30Z

The PR looks much better overall, but I think we need a proper validation on what the user is pasting, to properly manage the case when the input is not convertible into a number.

You don't think the regex string matching is enough? Currently if the value can't be formatted into a number then it is a no-op and doesn't allow it to be pasted.

ok I think that might be good enough

MaxwellDG added 2 commits November 29, 2023 12:42

feat(amount-input-screen): allow for pasting values in amount input s…

e94771e

…creen

refactor: eslint

bf31e5a

nicolasburtey reviewed Dec 1, 2023

View reviewed changes

app/components/amount-input-screen/amount-input-screen-ui.tsx Outdated Show resolved Hide resolved

nicolasburtey reviewed Dec 1, 2023

View reviewed changes

MaxwellDG added 3 commits December 4, 2023 12:24

Merge branch 'main' into feature/input-currency-val

587ef3a

refactor: makin pasting its own action and better type safety

3a3d1cf

refactor: linting

e3031ed

MaxwellDG requested a review from nicolasburtey December 5, 2023 07:38

nicolasburtey reviewed Dec 6, 2023

View reviewed changes

MaxwellDG added 2 commits December 6, 2023 16:15

fix: Removed type 'Keys'

3606259

fix: added trim to pasted input

a7a5a2f

nicolasburtey approved these changes Dec 8, 2023

View reviewed changes

nicolasburtey merged commit 0d49abe into GaloyMoney:main Dec 8, 2023
7 checks passed

MaxwellDG deleted the feature/input-currency-val branch December 8, 2023 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(amount-input-screen): allow for pasting values in amount input s… #2854

feat(amount-input-screen): allow for pasting values in amount input s… #2854

MaxwellDG commented Nov 29, 2023

nicolasburtey commented Dec 1, 2023 •

edited

Loading

nicolasburtey Dec 1, 2023

MaxwellDG Dec 1, 2023

MaxwellDG Dec 4, 2023 •

edited

Loading

nicolasburtey commented Dec 1, 2023

nicolasburtey Dec 6, 2023

MaxwellDG Dec 6, 2023

nicolasburtey Dec 6, 2023 •

edited

Loading

nicolasburtey Dec 6, 2023

MaxwellDG Dec 6, 2023 •

edited

Loading

MaxwellDG Dec 6, 2023

nicolasburtey commented Dec 6, 2023

MaxwellDG commented Dec 6, 2023 •

edited

Loading

nicolasburtey commented Dec 8, 2023

		@@ -54,6 +61,8 @@ export const Key = {

		export type Key = (typeof Key)[keyof typeof Key]

		export type Keys = `${number \| ""}${"." \| ""}${number \| ""}`

feat(amount-input-screen): allow for pasting values in amount input s… #2854

feat(amount-input-screen): allow for pasting values in amount input s… #2854

Conversation

MaxwellDG commented Nov 29, 2023

nicolasburtey commented Dec 1, 2023 • edited Loading

nicolasburtey Dec 1, 2023

Choose a reason for hiding this comment

MaxwellDG Dec 1, 2023

Choose a reason for hiding this comment

MaxwellDG Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

nicolasburtey commented Dec 1, 2023

nicolasburtey Dec 6, 2023

Choose a reason for hiding this comment

MaxwellDG Dec 6, 2023

Choose a reason for hiding this comment

nicolasburtey Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

nicolasburtey Dec 6, 2023

Choose a reason for hiding this comment

MaxwellDG Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

MaxwellDG Dec 6, 2023

Choose a reason for hiding this comment

nicolasburtey commented Dec 6, 2023

MaxwellDG commented Dec 6, 2023 • edited Loading

nicolasburtey commented Dec 8, 2023

nicolasburtey commented Dec 1, 2023 •

edited

Loading

MaxwellDG Dec 4, 2023 •

edited

Loading

nicolasburtey Dec 6, 2023 •

edited

Loading

MaxwellDG Dec 6, 2023 •

edited

Loading

MaxwellDG commented Dec 6, 2023 •

edited

Loading