Reusable and stylable money request amount input #40390

ZhenjaHorbach · 2024-04-18T02:03:18Z

Details

Reusable and stylable money request amount input

Fixed Issues

$ #40382
PROPOSAL: #40382 (comment)

Tests

Open App
Click on FAB button
Choose Submit expence
Verify that Money Request Amount Input works correctly
After verifying open account settings and choose Profile
Click on Status, click on clear after, choose custom and click on Time
Verify that Time Picker works correctly

Verify that no errors appear in the JS console

Offline tests

Open App
Click on FAB button
Choose Submit expence
Verify that Money Request Amount Input works correctly
After verifying open account settings and choose Profile
Click on Status, click on clear after, choose custom and click on Time
Verify that Time Picker works correctly

QA Steps

Open App
Click on FAB button
Choose Submit expence
Verify that Money Request Amount Input works correctly
After verifying open account settings and choose Profile
Click on Status, click on clear after, choose custom and click on Time
Verify that Time Picker works correctly

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

android-web.mov

Android: mWeb Chrome

android.mov

iOS: Native

ios.mov

iOS: mWeb Safari

ios-web_0kuJlBMd.mp4

MacOS: Chrome / Safari

web_6rw0KoPH.mp4

MacOS: Desktop

desktop_MNbi88xu.mp4

melvin-bot · 2024-04-18T02:03:23Z

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

melvin-bot · 2024-04-18T02:03:23Z

@hungvu193 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

hungvu193 · 2024-04-18T02:17:13Z

Please let's me know when this is ready. Thank you 😄

ZhenjaHorbach · 2024-04-18T02:27:49Z

Please let's me know when this is ready. Thank you 😄

I think the PR is already ready)
Maybe it could be made a little cleaner

But I need some sleep
I'll come back in a couple of hours and fill out the checklist and fix comments If they will)

hungvu193 · 2024-04-18T02:46:03Z

Please let's me know when this is ready. Thank you 😄

I think the PR is already ready)

Maybe it could be made a little cleaner

But I need some sleep

I'll come back in a couple of hours and fill out the checklist and fix comments If they will)

Sure. Get some rests 😄 . I'll review in the mean time

dubielzyk-expensify · 2024-04-18T03:57:56Z

Would be good to get some screenshots too.

And just to confirm that we're not messing with the defaults of the input fields but rather adding a variation?

cc @youssef-lr @Expensify/design

ZhenjaHorbach · 2024-04-18T08:18:42Z

Please let's me know when this is ready. Thank you 😄

I think the PR is already ready)
Maybe it could be made a little cleaner
But I need some sleep
I'll come back in a couple of hours and fill out the checklist and fix comments If they will)

Sure. Get some rests 😄 . I'll review in the mean time

Hello )
I updated checklist
So PR is completely ready

ZhenjaHorbach · 2024-04-18T08:19:33Z

Would be good to get some screenshots too.

And just to confirm that we're not messing with the defaults of the input fields but rather adding a variation?

cc @youssef-lr @Expensify/design

@dubielzyk-expensify

Hello )
I made videos
The old inputs remain the same

ZhenjaHorbach · 2024-04-18T08:20:54Z

@youssef-lr

Hello)
I made some screenshots
To show how it will look in your PR

Plus I add prop for prefix and prefixContainer to make this component more flexible

hungvu193 · 2024-04-18T08:21:35Z

Hello )

I update checklist

So PR is completely ready )

Cool. I'm AFK now but I'll review in the next couples of hour

shawnborton · 2024-04-18T10:11:45Z

Is it possible to right-align the text AND the prefixed character, while allowing it to be flexible enough to grow if needed?

ZhenjaHorbach · 2024-04-18T11:11:07Z

Is it possible to right-align the text AND the prefixed character, while allowing it to be flexible enough to grow if needed?

I'll test it now

ZhenjaHorbach · 2024-04-18T11:22:17Z

Is it possible to right-align the text AND the prefixed character, while allowing it to be flexible enough to grow if needed?

@shawnborton
Is it okay ?)

2024-04-18.13.20.18.mov

But in any case, I think at the moment we have a very flexible input
Because we can add styles for:
-input;
-inputContainer;
-prefix;
-prefixContainer

shawnborton · 2024-04-18T11:49:09Z

That does feel pretty good... can you show it using USD $, as I think that will be the mainline case? Thanks!

ZhenjaHorbach · 2024-04-18T12:02:03Z

That does feel pretty good... can you show it using USD $, as I think that will be the mainline case? Thanks!

here it is )

2024-04-18.14.00.13.mov

I used minimum width for examples
But if any case we can remove it

2024-04-18.14.00.53.mov

shawnborton · 2024-04-18T12:21:38Z

It feels like quite a big gap between the currency symbol and the amount here:

ZhenjaHorbach · 2024-04-18T12:28:55Z

It feels like quite a big gap between the currency symbol and the amount here:

What about this ?
I removed minWidth

shawnborton · 2024-04-18T12:37:53Z

Is there a way we can give the inputs a minWidth but also have the prefixed $ just sit right next to the amount? This is the mockup we created for this particular feature:

ZhenjaHorbach · 2024-04-18T12:44:50Z

Is there a way we can give the inputs a minWidth but also have the prefixed $ just sit right next to the amount? This is the mockup we created for this particular feature:

I think it's not a problem )

src/components/MoneyRequestAmountInput.tsx

shawnborton · 2024-04-22T12:09:36Z

Let us know when this is ready for another design review!

youssef-lr · 2024-04-22T20:22:25Z

@shawnborton I have tested all inputs and everything looks good by default, for the split page style we can tackle those in my PR. We just need to ensure this PR is not affecting any default styles. I think let's go ahead and make another build!

shawnborton · 2024-04-22T20:26:40Z

Sounds good, will build now!

youssef-lr · 2024-04-22T20:48:39Z

src/components/MoneyRequestAmountInput.tsx

+            return;
+        }
+        const frontendAmount = amount ? CurrencyUtils.convertToFrontendAmount(amount).toString() : '';
+        setNewAmount(frontendAmount);


Suggested change

setNewAmount(frontendAmount);

setCurrentAmount(frontendAmount);

We need to use setCurrentAmount here as used previously. We shouldn't be triggering onAmountChange from here because that's supposed to be only triggered via user input and via setNewAmount

Nice catch on this one 👍, I should have caught it earlier 🤦

github-actions · 2024-04-22T20:50:05Z

🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪

Android 🤖	iOS 🍎
❌ FAILED ❌	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/40390/index.html
The QR code can't be generated, because the android build failed
Desktop 💻	Web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/40390/NewExpensify.dmg	https://40390.pr-testing.expensify.com

👀 View the workflow run that generated this build 👀

youssef-lr · 2024-04-22T23:27:15Z

@shawnborton let me know how the testing goes! I haven't found any issue in my testing so I think we're very close to merge this!

dubielzyk-expensify · 2024-04-23T00:25:37Z

Not sure if this was introduced in this PR but the button has no padding on the bottom. This doesn't happen in Prod/Staging on the status page, but it happens on Wallet on both (I'll file a bug separately):

dubielzyk-expensify · 2024-04-23T00:25:49Z

Input wise I couldn't find any issues

hungvu193 · 2024-04-23T01:33:14Z

Not sure if this was introduced in this PR but the button has no padding on the bottom. This doesn't happen in Prod/Staging on the status page, but it happens on Wallet on both (I'll file a bug separately):

This is known issue and was reported earlier by @shawnborton in this slack convo:
https://expensify.slack.com/archives/C01GTK53T8Q/p1713455630242319

youssef-lr · 2024-04-23T07:31:57Z

Thanks for testing @dubielzyk-expensify! I'll wait to hear from @shawnborton before merging.

shawnborton · 2024-04-23T12:55:40Z

Gonna start testing this shortly, will report back!

shawnborton · 2024-04-23T16:09:30Z

Did some testing on iOS and couldn't seem to find anything, so I'd say this is all good on my end 👍

youssef-lr · 2024-04-23T16:29:29Z

Awesome, thanks everyone!!

OSBotify · 2024-04-23T16:40:30Z

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

OSBotify · 2024-04-24T00:07:32Z

🚀 Deployed to staging by https://github.com/youssef-lr in version: 1.4.65-0 🚀

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

OSBotify · 2024-04-25T13:45:51Z

🚀 Deployed to production by https://github.com/mountiny in version: 1.4.65-5 🚀

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

jayeshmangwani · 2024-06-19T11:06:01Z

src/components/MoneyRequestAmountInput.tsx

+            }}
+            selectedCurrencyCode={currency}
+            selection={selection}
+            onSelectionChange={(e: NativeSyntheticEvent<TextInputSelectionChangeEventData>) => {


Coming from this issue #41762 , we are encountering an edge case on Android. When we select all the number and then add a number, the cursor is misplaced and appears at the start of the number. The reason for this is that on Android, when we select all the text and add a number, the selection update from onSelectionChange is triggered earlier than the manual selection update. As a result, the prevSelection already contains the new selection, leading to an incorrect selection calculation more context on the proposal #41762 (comment)

rayane-d · 2024-11-06T12:40:20Z

src/components/MoneyRequestAmountInput.tsx

+            }}
+            selectedCurrencyCode={currency}
+            selection={selection}
+            onSelectionChange={(e: NativeSyntheticEvent<TextInputSelectionChangeEventData>) => {


FYI, this caused the following issue: #50886
Detailed RCA can be found in the description of this PR: #51306 and in this comment: #50886 (comment)

ZhenjaHorbach added 5 commits April 18, 2024 01:56

Create draft MoneyRequestAmountInput

f799c08

Fix bug with styles

aa5c56d

Move MoneyRequestAmountInput to components

3aeb011

Refactor MoneyRequestAmountInput

727a554

Update styles for new component

9f6d574

ZhenjaHorbach requested a review from a team as a code owner April 18, 2024 02:03

melvin-bot bot requested review from hungvu193 and removed request for a team April 18, 2024 02:03

Refactor styles for MoneyRequestAmountInput

dcafc98

ZhenjaHorbach requested a review from a team as a code owner April 18, 2024 02:25

Add prefixContainerStyle

4e8cd5e

youssef-lr reviewed Apr 19, 2024

View reviewed changes

src/components/MoneyRequestAmountInput.tsx Show resolved Hide resolved

ZhenjaHorbach added 2 commits April 20, 2024 01:20

Fix comments

c3ff30f

Remove unnecessary spaces

06b51b1

youssef-lr reviewed Apr 22, 2024

View reviewed changes

Update function for changing currentAmount after update amount

a0176b5

youssef-lr mentioned this pull request Apr 22, 2024

[HOLD] Implement Uneven Splits #40386

Merged

59 tasks

youssef-lr approved these changes Apr 23, 2024

View reviewed changes

youssef-lr merged commit 058044b into Expensify:main Apr 23, 2024
17 checks passed

github-actions bot mentioned this pull request Apr 23, 2024

Deploy Checklist: New Expensify 2024-04-23 #40843

Closed

74 tasks

kbecciv mentioned this pull request Apr 24, 2024

iOS - Status - Cursor jumps behind the minutes after entering hour #40930

Closed

6 tasks

melvin-bot bot mentioned this pull request Apr 25, 2024

[HOLD for payment 2024-05-02] [$750] [CRITICAL] Create a reusable and stylable MoneyRequestAmountInput #40382

Closed

jayeshmangwani reviewed Jun 19, 2024

View reviewed changes

jayeshmangwani mentioned this pull request Jun 19, 2024

[HOLD for payment 2024-06-20] [$250] [Uneven Splits] [Polish] Android - Cursor appears in front of digit when highlighting amount and entering digit #41762

Closed

6 tasks

rayane-d reviewed Nov 6, 2024

View reviewed changes

rayane-d mentioned this pull request Nov 11, 2024

[HOLD for payment 2024-11-11] iOS - Hybrid app - Split - Cursor jumps in front of digit when deleting participant's amount #50886

Closed

8 tasks

	setNewAmount(frontendAmount);
	setCurrentAmount(frontendAmount);

Reusable and stylable money request amount input #40390

Reusable and stylable money request amount input #40390

Conversation

ZhenjaHorbach commented Apr 18, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Apr 18, 2024

melvin-bot bot commented Apr 18, 2024

hungvu193 commented Apr 18, 2024

ZhenjaHorbach commented Apr 18, 2024 • edited Loading

hungvu193 commented Apr 18, 2024 • edited Loading

dubielzyk-expensify commented Apr 18, 2024

ZhenjaHorbach commented Apr 18, 2024 • edited Loading

ZhenjaHorbach commented Apr 18, 2024 • edited Loading

ZhenjaHorbach commented Apr 18, 2024 • edited Loading

hungvu193 commented Apr 18, 2024

shawnborton commented Apr 18, 2024

ZhenjaHorbach commented Apr 18, 2024

ZhenjaHorbach commented Apr 18, 2024

shawnborton commented Apr 18, 2024

ZhenjaHorbach commented Apr 18, 2024 • edited Loading

shawnborton commented Apr 18, 2024 • edited Loading

ZhenjaHorbach commented Apr 18, 2024 • edited Loading

shawnborton commented Apr 18, 2024

ZhenjaHorbach commented Apr 18, 2024 • edited Loading

shawnborton commented Apr 22, 2024

youssef-lr commented Apr 22, 2024

shawnborton commented Apr 22, 2024

youssef-lr Apr 22, 2024

Choose a reason for hiding this comment

ZhenjaHorbach Apr 22, 2024

Choose a reason for hiding this comment

hungvu193 Apr 23, 2024

Choose a reason for hiding this comment

github-actions bot commented Apr 22, 2024

youssef-lr commented Apr 22, 2024

dubielzyk-expensify commented Apr 23, 2024

dubielzyk-expensify commented Apr 23, 2024

hungvu193 commented Apr 23, 2024 • edited Loading

youssef-lr commented Apr 23, 2024

shawnborton commented Apr 23, 2024

shawnborton commented Apr 23, 2024

youssef-lr commented Apr 23, 2024

OSBotify commented Apr 23, 2024

OSBotify commented Apr 24, 2024

OSBotify commented Apr 25, 2024

jayeshmangwani Jun 19, 2024

Choose a reason for hiding this comment

rayane-d Nov 6, 2024

Choose a reason for hiding this comment

ZhenjaHorbach commented Apr 18, 2024 •

edited

Loading

ZhenjaHorbach commented Apr 18, 2024 •

edited

Loading

hungvu193 commented Apr 18, 2024 •

edited

Loading

ZhenjaHorbach commented Apr 18, 2024 •

edited

Loading

ZhenjaHorbach commented Apr 18, 2024 •

edited

Loading

ZhenjaHorbach commented Apr 18, 2024 •

edited

Loading

ZhenjaHorbach commented Apr 18, 2024 •

edited

Loading

shawnborton commented Apr 18, 2024 •

edited

Loading

ZhenjaHorbach commented Apr 18, 2024 •

edited

Loading

ZhenjaHorbach commented Apr 18, 2024 •

edited

Loading

hungvu193 commented Apr 23, 2024 •

edited

Loading