Refactor IOUAmountPage #2203

tugbadogan · 2021-04-01T20:33:37Z

@trjExpensify @Julesssss @tgolen

Details

Refactor IOUAmountPage as we discussed in Fixed moving cursor to end problem except IOUAmountPage #2010
Moved TextInputAutoGrow to its own component
Moved inline styles to styles library as @marcaaron recommended.

Fixed Issues

Fixes #1691

Tests

Manually tested on all platforms.

QA Steps

Go to the following URL: http://localhost:8080/iou/split
Make sure you can write only decimal number(unlimited digits and up to 3 decimal numbers) to input field.
Make sure chat compose input is working as usual.

Tested On

Screenshots

Web

Screen.Recording.2021-04-01.at.17.46.06.mov

Android

Screen.Recording.2021-04-01.at.17.44.41.mov

Merge

src/components/TextInputAutoGrow.js

src/pages/iou/steps/IOUAmountPage.js

src/styles/styles.js

This change makes input entry smoother by preventing TextInput to update its own value

Merge

shawnborton · 2021-04-05T23:16:19Z

Couple of design comments:

It looks like the IOU amount is in black, but it should be in themeColors.heading
It looks like the background color for the number pad buttons is missing. They should just use our default button styles.

tgolen

Looks like there are some conflicts

src/components/TextInputAutoGrow.js

src/components/TextInputFocusable/index.js

src/pages/iou/steps/IOUAmountPage.js

Julesssss · 2021-04-07T14:30:12Z

src/styles/styles.js

+function getHiddenElementOutsideOfWindow(windowWidth) {
+    return {
+        position: 'absolute',
+        opacity: 0,
+        left: windowWidth,
+    };
+}


Are there any alternatives to this?

I wanted to move this hidden element out of the screen so it doesn't block any visible component. Maybe we can try using z-index and move this component background.

I'd be interested in seeing if a negative z-index gives us a slightly simpler solution, but I don't think it should block the PR.

there is also transform: translateX() as an option

I used transform solution to hide the element like this:

hiddenElementOutsideOfWindow: { position: 'fixed', top: 0, left: 0, opacity: 0, transform: 'translateX(-100%)', },

Merge

tugbadogan · 2021-04-12T16:00:02Z

@shawnborton

Couple of design comments:

It looks like the IOU amount is in black, but it should be in themeColors.heading

I updated IOU Amount color.

It looks like the background color for the number pad buttons is missing. They should just use our default button styles.

I'm already using styles.button style for number pad buttons. Am I missing something here? They look like design screenshots in the issue.

https://github.com/Expensify/Expensify.cash/blob/d3f1d28478357e2a66b155e660def191d6ed79a8/src/components/BigNumberPad.js#L49-L53

shawnborton · 2021-04-12T16:28:45Z

From the video you posted, it looks like the buttons do not have a background color:

But perhaps that's just the video not showing all of the colors.

tgolen · 2021-04-12T22:01:06Z

Can you please fix the conflicts before I take another look at this?

Merge

tugbadogan · 2021-04-13T17:08:13Z

Can you please fix the conflicts before I take another look at this?

@tgolen
I resolved the conflicts. Can you review again?

marcaaron

Mostly looks good. Only have some small comments about naming conventions.

src/components/TextInputAutoGrow.js

src/pages/iou/steps/IOUAmountPage.js

tugbadogan · 2021-04-14T14:01:01Z

@tgolen I updated the PR. Can you review again?

src/styles/styles.js

marcaaron · 2021-04-14T18:06:07Z

I believe this PR may have caused a regression related to many text inputs.
In some cases, they become completely unresponsive... cc @stitesExpensify

marcaaron · 2021-04-14T18:17:48Z

The problem is that if we have no value or defaultValue passed then it will pass a prop of value: "" to a TextInput

tugbadogan · 2021-04-14T18:27:47Z

Thanks for the flag @marcaaron , creating a PR now.

isagoico · 2021-04-14T19:12:51Z

I don't think we're able to QA this PR since it requires changing some code. Can you guys assign someone to QA and let me know once it's done so we can check it off the list? Thanks in advance. CC @marcaaron @tgolen @Julesssss @ctkochan22

marcaaron · 2021-04-14T19:17:19Z

@isagoico Seems testable by using a deep link or entering url in web? Which code changes are you referring to?

isagoico · 2021-04-14T19:31:16Z

Oh idk why I thought there was a code change needed here 🤦 my apologies.

Go to the following URL: http://localhost:8080/iou/split

Can you share the deeplink or URL that we can use to test this?

marcaaron · 2021-04-14T19:39:54Z

I believe https://staging.expensify.cash/iou/split should work.

isagoico · 2021-04-14T20:10:44Z

I think that worked.

Should we test this only on web and mWeb? Not sure if we're able to navigate to that URL or feature in the apps (android, iOS, dekstop)

marcaaron · 2021-04-14T20:14:24Z

It should work on all platforms except desktop I think?

isagoico · 2021-04-14T20:18:15Z

~~Do you have steps on how can we reach this "Split bill" page on android and iOS apps?~~ Ignore me :) I figured it out with the comment in the other PR. Thanks!

isagoico · 2021-04-14T22:33:06Z

Split - Able to enter more than 8 decimals and LHN/conversation area is blank

Expected Result

Make sure you can write only decimal number(max 6 digit and 2 decimal) to input field.

Actual Result

Able to enter 3 decimal inputs

Action performed

Launch the url and login
Add iou/split to the URL
Input decimal in the field

Platform

Issue is confirmed in:

Android ✔️
Web ✔️

Build version: 1.0.21-0

Notes/Images/Video

marcaaron · 2021-04-14T22:38:09Z

I think this test step is out of date and needed to be updated.

tugbadogan · 2021-04-14T22:41:20Z

Sorry, I forgot to update the test plan. Fixed it now.

OSBotify · 2021-04-19T22:41:56Z

🚀 [Deployed](https://github.com/Expensify/Expensify.cash
/actions/runs/765130740) 🚀 to
staging on Mon Apr 19 2021 at 22:41:56 GMT+0000 (Coordinated Universal Time)

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

tugbadogan and others added 7 commits March 20, 2021 21:33

Merge pull request #4 from Expensify/master

c5bee66

Merge

Merge pull request #5 from Expensify/master

1b1526f

Merge

Merge pull request #6 from Expensify/master

fa6552d

Merge

Merge pull request #7 from Expensify/master

cf9d603

Merge

Merge pull request #8 from Expensify/master

ae30f03

Merge

Refactor IOUAmountPage

cdc67f2

Move style object to styles

37deb3e

tugbadogan requested a review from a team as a code owner April 1, 2021 20:33

MelvinBot requested review from ctkochan22 and removed request for a team April 1, 2021 20:33

Merge pull request #9 from Expensify/master

b140d36

Merge

marcaaron requested changes Apr 1, 2021

View reviewed changes

marcaaron requested a review from Julesssss April 1, 2021 21:34

marcaaron assigned tugbadogan Apr 1, 2021

tugbadogan and others added 3 commits April 2, 2021 13:40

Addressing comments

aa5ef6a

Use event.preventDefault in TextInputAutoGrow onKeyPress

359eb3e

This change makes input entry smoother by preventing TextInput to update its own value

Merge pull request #10 from Expensify/master

28b7fef

Merge

tgolen requested changes Apr 6, 2021

View reviewed changes

Julesssss reviewed Apr 7, 2021

View reviewed changes

tugbadogan added 2 commits April 8, 2021 16:28

Merge pull request #11 from Expensify/master

743fce8

Merge

Merge pull request #12 from Expensify/master

8440e0c

Merge

Addressing comments

6a382c1

tugbadogan and others added 2 commits April 13, 2021 16:41

Merge pull request #13 from Expensify/master

4dfdbd2

Merge

Merge branch 'master' into tugbadogan-refactor-iou-amount-page

758c441

tgolen previously approved these changes Apr 13, 2021

View reviewed changes

Julesssss previously approved these changes Apr 13, 2021

View reviewed changes

marcaaron requested changes Apr 13, 2021

View reviewed changes

src/components/TextInputAutoGrow.js Outdated Show resolved Hide resolved

src/components/TextInputAutoGrow.js Outdated Show resolved Hide resolved

src/pages/iou/steps/IOUAmountPage.js Outdated Show resolved Hide resolved

Addressing comments

a8e4b14

tugbadogan dismissed stale reviews from Julesssss and tgolen via a8e4b14 April 14, 2021 13:53

Updating regex comment

afddb0f

Julesssss reviewed Apr 14, 2021

View reviewed changes

src/styles/styles.js Show resolved Hide resolved

Julesssss approved these changes Apr 14, 2021

View reviewed changes

marcaaron approved these changes Apr 14, 2021

View reviewed changes

marcaaron merged commit 4f9da28 into Expensify:master Apr 14, 2021

OSBotify mentioned this pull request Apr 14, 2021

Deploy Checklist: Expensify.cash 2021-04-13 #2380

Closed

66 tasks

tugbadogan mentioned this pull request Apr 14, 2021

Fix unresponsive TextInputFocusable #2396

Closed

5 tasks

tugbadogan deleted the tugbadogan-refactor-iou-amount-page branch April 14, 2021 18:38

marcaaron mentioned this pull request Apr 14, 2021

Let parent component handle defaultValue and value in TextInputFocusable #2398

Merged

5 tasks

isagoico mentioned this pull request Apr 14, 2021

iOS - App option not available when tapping deep link #2401

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor IOUAmountPage #2203

Refactor IOUAmountPage #2203

tugbadogan commented Apr 1, 2021 •

edited

Loading

shawnborton commented Apr 5, 2021

tgolen left a comment

Julesssss Apr 7, 2021

tugbadogan Apr 13, 2021

Julesssss Apr 13, 2021

marcaaron Apr 13, 2021

tugbadogan Apr 14, 2021

tugbadogan commented Apr 12, 2021

shawnborton commented Apr 12, 2021

tgolen commented Apr 12, 2021

tugbadogan commented Apr 13, 2021

marcaaron left a comment

tugbadogan commented Apr 14, 2021

marcaaron commented Apr 14, 2021

marcaaron commented Apr 14, 2021

tugbadogan commented Apr 14, 2021

isagoico commented Apr 14, 2021

marcaaron commented Apr 14, 2021

isagoico commented Apr 14, 2021 •

edited

Loading

marcaaron commented Apr 14, 2021 •

edited

Loading

isagoico commented Apr 14, 2021

marcaaron commented Apr 14, 2021

isagoico commented Apr 14, 2021 •

edited

Loading

isagoico commented Apr 14, 2021

marcaaron commented Apr 14, 2021

tugbadogan commented Apr 14, 2021

OSBotify commented Apr 19, 2021

Refactor IOUAmountPage #2203

Refactor IOUAmountPage #2203

Conversation

tugbadogan commented Apr 1, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Android

shawnborton commented Apr 5, 2021

tgolen left a comment

Choose a reason for hiding this comment

Julesssss Apr 7, 2021

Choose a reason for hiding this comment

tugbadogan Apr 13, 2021

Choose a reason for hiding this comment

Julesssss Apr 13, 2021

Choose a reason for hiding this comment

marcaaron Apr 13, 2021

Choose a reason for hiding this comment

tugbadogan Apr 14, 2021

Choose a reason for hiding this comment

tugbadogan commented Apr 12, 2021

shawnborton commented Apr 12, 2021

tgolen commented Apr 12, 2021

tugbadogan commented Apr 13, 2021

marcaaron left a comment

Choose a reason for hiding this comment

tugbadogan commented Apr 14, 2021

marcaaron commented Apr 14, 2021

marcaaron commented Apr 14, 2021

tugbadogan commented Apr 14, 2021

isagoico commented Apr 14, 2021

marcaaron commented Apr 14, 2021

isagoico commented Apr 14, 2021 • edited Loading

marcaaron commented Apr 14, 2021 • edited Loading

isagoico commented Apr 14, 2021

marcaaron commented Apr 14, 2021

isagoico commented Apr 14, 2021 • edited Loading

isagoico commented Apr 14, 2021

Split - Able to enter more than 8 decimals and LHN/conversation area is blank

Expected Result

Actual Result

Action performed

Platform

marcaaron commented Apr 14, 2021

tugbadogan commented Apr 14, 2021

OSBotify commented Apr 19, 2021

tugbadogan commented Apr 1, 2021 •

edited

Loading

isagoico commented Apr 14, 2021 •

edited

Loading

marcaaron commented Apr 14, 2021 •

edited

Loading

isagoico commented Apr 14, 2021 •

edited

Loading