Add Push-to-Page support for IOUConfirmationList #16919

yuwenmemon · 2023-04-04T16:02:12Z

cc @luacmartins @vitHoracek

Details

Change the IOUConfirmationList form to use push to page components and spruce up the look of it a bit.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/270489

Tests / QA

Start a Send money flow.
Select an amount, and recipient and get to the final confirmation page
Edit the "amount" field. Make sure you are taken back to the first step
Click next after editing the amount, make sure you are taken back to the confirmation page
Click the "back" button, make sure you are taken to the page where you select a recipient.
Select another recipient, make sure you are taken back to the confirmation page.
Click "What's it for" - add a description
Click the "Pay with Expensify" button at the bottom. make sure the amount and description show up on the corresponding IOU expense.

Start a Request money flow.
Select an amount, and recipient and get to the final confirmation page
Edit the "amount" field. Make sure you are taken back to the first step
Click next after editing the amount, make sure you are taken back to the confirmation page
Click the "back" button, make sure you are taken to the page where you select a recipient.
Select another recipient, make sure you are taken back to the confirmation page.
Click "What's it for" - add a description
Click the Request $XX.XX button at the bottom. make sure the amount and description show up on the corresponding IOU expense.

Start a Split Bill flow.
Select an amount, and recipient and get to the final confirmation page
Edit the "amount" field. Make sure you are taken back to the first step
Click next after editing the amount, make sure you are taken back to the confirmation page
Click the "back" button, make sure you are taken to the page where you select a recipient.
Select another recipient, make sure you are taken back to the confirmation page.
Click "What's it for" - add a description
Click the button at the bottom. make sure the amount and description show up on the corresponding IOU expense.

Verify that no errors appear in the JS console in any of the above tests

Offline tests

Do the request money and split bill flows but offline. Make sure they work the same except with a pending transaction created.

PR Author Checklist

Screenshots/Videos

Web

Kapture.2023-04-14.at.00.11.51.mp4

Mobile Web - Chrome

Kapture.2023-04-10.at.18.29.53.mp4

Mobile Web - Safari

Kapture.2023-04-10.at.18.19.48.mp4

Desktop

Kapture.2023-04-10.at.17.53.22.mp4

iOS

Kapture.2023-04-10.at.18.32.55.mp4

Android

Kapture.2023-04-10.at.18.20.58.mp4

Julesssss

The styling looks good so far. Will run through the full tests once complete.

(not sure why I pulled this branch for review given that it's WIP, but I'm here now anyway)

src/components/IOUConfirmationList.js

mountiny

There was a recent change to the design where also the description will be push to page for consistency https://expensify.slack.com/archives/C02HWMSMZEC/p1680719627779599

MelvinBot · 2023-04-10T21:53:05Z

@thesahindia @MonilBhavsar One of you needs to 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]

…IOUConfirmationList

yuwenmemon · 2023-04-10T22:48:12Z

This is ready for review!

mountiny

@yuwenmemon Great job! The main thing here is to update the What's it for? to Description to align with the doc

mountiny · 2023-04-11T12:11:25Z

src/pages/iou/IOUDescriptionPage.js

+                    <View style={styles.mb4}>
+                        <TextInput
+                            inputID="iouComment"
+                            name="ioucomment"


Is this lowercase on purpose?

Suggested change

name="ioucomment"

name="iouComment"

mountiny · 2023-04-11T12:13:16Z

src/pages/iou/IOUDescriptionPage.js

+        return (
+            <ScreenWrapper includeSafeAreaPaddingBottom={false}>
+                <HeaderWithCloseButton
+                    title={this.props.translate('iOUConfirmationList.whatsItFor')}


This should be Description

Suggested change

title={this.props.translate('iOUConfirmationList.whatsItFor')}

title={this.props.translate('IOUConfirmationList.description')}

mountiny · 2023-04-11T12:13:39Z

src/libs/actions/IOU.js

@@ -636,6 +636,15 @@ function setIOUSelectedCurrency(selectedCurrencyCode) {
    Onyx.merge(ONYXKEYS.IOU, {selectedCurrencyCode});
 }

+/**
+ * Sets IOU'S comment


Suggested change

* Sets IOU'S comment

* Sets IOU's comment

Can we change this comment to Sets Money Request description instead?

mountiny · 2023-04-11T12:15:29Z

src/components/IOUConfirmationList.js

+                <MenuItemWithTopDescription
+                    shouldShowRightIcon
+                    title={this.props.iou.comment}
+                    description={this.props.translate('iOUConfirmationList.whatsItFor')}


Lets update the key if its indeed with the typo and small i at start. We need to use Description here.

Suggested change

description={this.props.translate('iOUConfirmationList.whatsItFor')}

description={this.props.translate('IOUConfirmationList.description')}

I suggested that we rename these keys to moneyRequestConfirmationList in this PR

+1 to changing it to Description

Sounds good. What do you guys think about using "What's it for" in the hint for the text input?

Otherwise, it looks pretty bare:

Yeah I think we can keep that in there, good point

Sounds good!

src/pages/iou/steps/IOUConfirmPage.js

mountiny · 2023-04-11T17:12:06Z

I am wondering here @luacmartins @Julesssss the Amount edit is now navigating to a different step in the IOU creation flow so it oes not have its route same as other push-to-page edits in forms. I feel like its inconsistent, but I am not fully sure either if we should create a new edit mount page same as for the description field. It makes sense we should do it, any reason why not?

luacmartins

Looking good. Left a few comments, let's keep up the momentum here!

src/pages/iou/IOUDescriptionPage.js

src/components/OptionsSelector/BaseOptionsSelector.js

src/libs/Navigation/AppNavigator/ModalStackNavigators.js

src/styles/styles.js

src/components/IOUConfirmationList.js

luacmartins · 2023-04-11T19:11:13Z

src/components/IOUConfirmationList.js

+                <MenuItemWithTopDescription
+                    shouldShowRightIcon
+                    title={this.props.iou.comment}
+                    description={this.props.translate('iOUConfirmationList.whatsItFor')}


+1 to changing it to Description

src/pages/iou/IOUDescriptionPage.js

luacmartins · 2023-04-11T19:12:14Z

src/pages/iou/IOUDescriptionPage.js

+    },
+};
+
+class IOUDescriptionPage extends Component {


NAB should we create this as a functional component and use hooks already?

I'm not up to speed on hooks fully so I didn't go for it just for the sake of time. What do you think? Maybe a follow-up?

Seems fine, but I'm still not sure why this is a class component since we don't have any state or lifecycle methods. Seems like we could refactor to a functional component without the need for hooks.

…IOUConfirmationList

luacmartins

LGTM!

mountiny

Great, let's ship it!

Yuwen is definitely not standing behind me, looking over my shoulder making sure it gets merged.

OSBotify · 2023-04-14T23:55:31Z

✋ 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 · 2023-04-18T04:39:36Z

🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.1-0 🚀

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

OSBotify · 2023-04-18T06:09:12Z

🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.1-0 🚀

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

OSBotify · 2023-04-19T17:22:21Z

🚀 Deployed to production by https://github.com/mountiny in version: 1.3.1-3 🚀

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

eVoloshchak · 2023-05-05T17:14:57Z

This PR has caused a regression in #17656

I this commit titleTextStyle is calculated with props.titleStyle instead of props.style. OfflineWithFeedback has a limitation on mobile, where it would apply strikethrough styling only to the first child, not recursively, which in this case means it was previously applied to props.style, but wasn't applied to props.titleStyle

Santhosh-Sellavel · 2023-05-08T15:19:16Z

src/libs/actions/IOU.js

+ * @param {String} comment
+ */
+function setMoneyRequestDescription(comment) {
+    Onyx.merge(ONYXKEYS.IOU, {comment});


we forgot to trim white spaces in the Description comment and it lead to this issue

sobitneupane · 2023-05-17T13:04:48Z

While implementing MoneyRequestDescriptionPage, we didn't autofocus the text input though there is only single input in the page.

Related Issue: #17579

Santhosh-Sellavel · 2023-06-03T19:30:45Z

This PR caused a regression here. We update other things but missed to include IOUCurrencySelection to flow more details on the issue/PR

marcaaron · 2023-06-07T16:44:36Z

Are we adding validation to this form? I feel like we added this for a good reason...

App/src/components/Form.js

Lines 132 to 134 in bd5db76

    
           if (!_.isObject(validationErrors)) { 
        
               throw new Error('Validate callback must return an empty object or an object with shape {inputID: error}'); 
        
           }

and then did this...

App/src/pages/iou/MoneyRequestDescriptionPage.js

Line 74 in 2fb8061

validate={() => ({})}

😂

marcaaron · 2023-06-07T16:45:11Z

Should we add a polish item for that? @mountiny @luacmartins

luacmartins · 2023-06-07T20:15:04Z

That field is not required, so I'm not sure what we'd validate. The validate callback is returning an empty object in that case.

marcaaron · 2023-06-09T02:38:08Z

We can update Form so that people don't have to pass a function that returns an empty object ?

luacmartins · 2023-06-09T15:42:07Z

Done - #20525

Santhosh-Sellavel · 2023-06-12T19:14:33Z

I think this PR caused this #19450 bug, or should been caught here.

yuwenmemon added 2 commits April 3, 2023 17:57

Aesthetic changes to IOUConfirmationList for manual requests

f57b97f

Repurpose style prop for menuitem to apply to pressable

a453d1e

yuwenmemon self-assigned this Apr 4, 2023

yuwenmemon changed the title ~~[WIP] Add Push-to-Page~~ [WIP] Add Push-to-Page support for IOUConfirmationList Apr 4, 2023

mountiny self-requested a review April 4, 2023 16:11

Julesssss reviewed Apr 6, 2023

View reviewed changes

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

mountiny requested changes Apr 6, 2023

View reviewed changes

yuwenmemon added 6 commits April 7, 2023 12:47

Fix conflcits:

11df2c7

Add push to page support for amount field

c6d4fca

Add description page for IOU requests

5e16bf5

JSX linting

31ab836

Add some comments

268459c

ESLint

bf026c1

yuwenmemon marked this pull request as ready for review April 10, 2023 21:52

yuwenmemon requested a review from a team as a code owner April 10, 2023 21:52

melvin-bot bot requested review from MonilBhavsar and thesahindia and removed request for a team April 10, 2023 21:53

Merge branch 'main' of github.com:Expensify/App into yuwen-pushToPage…

49116c3

…IOUConfirmationList

yuwenmemon changed the title ~~[WIP] Add Push-to-Page support for IOUConfirmationList~~ Add Push-to-Page support for IOUConfirmationList Apr 10, 2023

luacmartins self-requested a review April 11, 2023 09:55

mountiny requested changes Apr 11, 2023

View reviewed changes

luacmartins reviewed Apr 11, 2023

View reviewed changes

yuwenmemon added 3 commits April 11, 2023 13:59

Merge branch 'main' of github.com:Expensify/App into yuwen-pushToPage…

8c5df5e

…IOUConfirmationList

use Description instead of whats it for?

a295dd8

IOU -> MoneyRequest

3055e18

Hooky hooky hooks

26a177e

luacmartins approved these changes Apr 14, 2023

View reviewed changes

mountiny approved these changes Apr 14, 2023

View reviewed changes

mountiny merged commit 6ee5e7a into main Apr 14, 2023

mountiny deleted the yuwen-pushToPageIOUConfirmationList branch April 14, 2023 23:50

OSBotify mentioned this pull request Apr 17, 2023

Deploy Checklist: New Expensify 2023-04-17 #17546

Closed

53 tasks

kbecciv mentioned this pull request Apr 18, 2023

[HOLD for payment 2023-05-16] [$1000] Request/Split/Send - There is NO cursor in the description input field #17579

Closed

6 tasks

dhairyasenjaliya mentioned this pull request Apr 19, 2023

[HOLD for payment 2023-05-03] [$1000] White spaces accepted as Description #17485

Closed

6 tasks

Prince-Mendiratta mentioned this pull request Apr 20, 2023

[HOLD for payment 2023-05-16] [$1000] No hover effect on Amount and Description fields on new send money page #17635

Closed

6 tasks

PrashantMangukiya mentioned this pull request Apr 20, 2023

[HOLD #17662] [$1000] Double sliding animation on Send/Request money #17710

Closed

6 tasks

Santhosh-Sellavel mentioned this pull request May 5, 2023

[PAID] [$1000] Leading whitespaces aren’t truncated while saving email/phone number in new contact method page #17770

Closed

6 tasks

eVoloshchak mentioned this pull request May 5, 2023

[PAID] [$500] Workspace - Deleted Workspace offline is not crossed out #17656

Closed

6 tasks

Santhosh-Sellavel reviewed May 8, 2023

View reviewed changes

Santhosh-Sellavel mentioned this pull request Jun 3, 2023

[HOLD for payment 2023-06-01] [$1000] The currency is updated even without clicking the save button #18667

Closed

6 tasks

This was referenced Jun 9, 2023

[HOLD for payment 2023-06-21] Make Form's validate prop optional #20525

Closed

Make validate prop optional in Form #20526

Merged

Santhosh-Sellavel mentioned this pull request Jun 12, 2023

[HOLD for payment 2023-06-13] [$1000] Pressing enter key on the Description page adds the user to the Who was There page. #19450

Closed

6 tasks

	title={this.props.translate('iOUConfirmationList.whatsItFor')}
	title={this.props.translate('IOUConfirmationList.description')}

	description={this.props.translate('iOUConfirmationList.whatsItFor')}
	description={this.props.translate('IOUConfirmationList.description')}

Add Push-to-Page support for IOUConfirmationList #16919

Add Push-to-Page support for IOUConfirmationList #16919

Conversation

yuwenmemon commented Apr 4, 2023 • edited Loading

Details

Fixed Issues

Tests / QA

Offline tests

PR Author Checklist

Screenshots/Videos

Julesssss left a comment

Choose a reason for hiding this comment

mountiny left a comment

Choose a reason for hiding this comment

MelvinBot commented Apr 10, 2023

yuwenmemon commented Apr 10, 2023

mountiny left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mountiny commented Apr 11, 2023

luacmartins left a 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

luacmartins left a comment

Choose a reason for hiding this comment

mountiny left a comment

Choose a reason for hiding this comment

OSBotify commented Apr 14, 2023

OSBotify commented Apr 18, 2023

OSBotify commented Apr 18, 2023

OSBotify commented Apr 19, 2023

eVoloshchak commented May 5, 2023

Choose a reason for hiding this comment

sobitneupane commented May 17, 2023

Santhosh-Sellavel commented Jun 3, 2023

marcaaron commented Jun 7, 2023

marcaaron commented Jun 7, 2023

luacmartins commented Jun 7, 2023

marcaaron commented Jun 9, 2023

luacmartins commented Jun 9, 2023

Santhosh-Sellavel commented Jun 12, 2023

yuwenmemon commented Apr 4, 2023 •

edited

Loading