GBR and Settlement button for the receiver on the invoice report preview

[waterim]

Details

GBR and Settlement button for the receiver on the invoice report preview

Fixed Issues

$ #40437
PROPOSAL: N/A

Tests

1. Go to FAB
2. Select "Send invoice"
3. Send Invoice
4. Go to user who needs to pay this invoice
5. Verify that correct settlement button appears in preview and inside of the report header
   Designs:

• Verify that no errors appear in the JS console

Offline tests

Same as tests

QA Steps

None

• Verify that no errors appear in the JS console

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • 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 followed proper code patterns (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 the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
  • 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 is localized by adding it to src/languages/* files and using the translation method
    • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review 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 followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• 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
• I verified that if a function's arguments changed that all usages have also been updated correctly
• 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 */
  • 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
  • If we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes
  • 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 any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• 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 code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
• 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.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

FOCUSED:

[rezkiy37]

In general, looks good!

[melvin-bot]

@ikevin127 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]

[shawnborton]

Let us know when there are screenshots to review, thanks!

[ikevin127]

Let me know whether my C+ review is actually needed here and if so, some testing steps would be helpful.
cc @cristipaval @waterim

[waterim]

@shawnborton Updated
new modals I will add in a separate PR

[shawnborton]

cc @davidcardoza @danielrvidal - can you check the screenshots in the original comment and let us know if they look good to you too?

This looks different from what we have in the doc, where we use the split button (drop down). Any reason why we wouldn't do that here too? I would think we'd use the split button but only have one option (pay as an individual). cc @cristipaval for thoughts.

[waterim]

cc @davidcardoza @danielrvidal - can you check the screenshots in the original comment and let us know if they look good to you too?

This looks different from what we have in the doc, where we use the split button (drop down). Any reason why we wouldn't do that here too? I would think we'd use the split button but only have one option (pay as an individual). cc @cristipaval for thoughts.

The dropdown appears when we have more than 1 options to pay
If we have individual or business it will be 2 options, but when we have only one individual only will appear, but I can add a "shouldAlwaysShowDropdownMenu" prop to always show a dropdown even if 1 option is there

[waterim]

@cristipaval Updated, deleted Business and Personal types and made payment method as elsewhere

[davidcardoza]

The dropdown appears when we have more than 1 options to pay
If we have individual or business it will be 2 options, but when we have only one individual only will appear, but I can add a "shouldAlwaysShowDropdownMenu" prop to always show a dropdown even if 1 option is there

• We should always display a dropdown menu, as this ensures a consistent design whether the user is paying as an individual or a business.
• The green pay button should simply state "Pay $XX.XX" and should not include "as an individual." When there are options to pay as a business, users will choose whether to pay as an individual or a business through a pay invoice flow. The green pay button will maintain a consistent design, reading "Pay $XX.XX" regardless of the selected payment method.

[waterim]

@davidcardoza Hello!
I was working on that issue to add required figma design, but as I almost finished with this view and this PR still open I can add just my changes here without doing it in a new PR, what do you think?

[VickyStash]

@waterim that's correct - My understanding is the behavior of that button would remain sticky after you've made your first payment (It will remember the last choice). But for the very first time, it might say [Pay $XX | 🔽] and clicking anywhere on the button (big side or small arrow side) would launch the dropdown popover. Let me know if that makes sense!

@shawnborton I'm not really sure if we should update the button to be split in this PR, because:

• Right now, the app doesn't save your default payment option for invoices, it's only planned functionality according to the Invoice Payments doc
• There is a separate ticket where button updates possibly will be handled, see the comment and the slack discussion

But I'm okay to do the updates in this ticket if you think it's better, just let me know

[shawnborton]

Those are totally valid points, I would be down to wait for the reasons you mentioned. Thoughts on that @davidcardoza? The tldr; Is that I suggested using the split button here, but we think we should wait for the invoice payments project for that.

[shawnborton]

For the title, we're close! Is it possible to make them the same height so that the version with the icon and the version without the icon feel identical? Basically this:

The icon is 20x20. So that means we just need to give the title a line height of 20px and we should maintain consistency on both spots.

[VickyStash]

Since the back item is rendering as a MenuItem component (that's an existing logic), a slightly different update was applied. That's how it looks.
@shawnborton what do you think?

1	2

w1.mp4

[shawnborton]

That feels really good to me!

For the mobile version, are we accunting for any kind of SafeArea padding we might need? Just wondering because it does seem like the last optionRow/button in the menu is kinda close to the home bar. Thoughts on that?

[VickyStash]

@shawnborton it looks like the additional padding exists:

Current feature	Main branch

[shawnborton]

Ah okay cool, I think we're all set then - but thank you for confirming!

[VickyStash]

@ikevin127 Could you please take a look at the PR?

[ikevin127]

Re-testing on all platforms, will return with videos soon!

[cristipaval]

Re-testing on all platforms, will return with videos soon!

Thanks a lot!

[ikevin127]

Caution

Everything looks good (see videos below), except there's one keyboard-related UX issue on all web based platforms:

The 2nd popup screen's [ < Individual ] go back button is not keyboard accessible (missing tab index ?). Therefore the user won't be able to go back to the popup's 1st page once they entered the 2nd page where the [ < Individual ] go back button is present.

The only thing the user can do with keyboard on the 2nd menu is to either Esc (close the popup) or Enter select one of the options under the [ < Individual ] go back button, which would also close the popup and perform the selected option action.

Keyboard UX Issue
web-kbd.mov

cc @VickyStash

Screenshots/Videos

Android: Native

android.webm

Android: mWeb Chrome

android-mweb.webm

iOS: Native

ios.mp4

iOS: mWeb Safari

ios-mweb.mp4

MacOS: Chrome / Safari

web.mov

MacOS: Desktop

desktop.mov

[VickyStash]

@ikevin127 The keyboard-focus issue you described is related to PopoverMenu component, and similar behavior can be reproduced on the main branch inside the video player:

• send a video as a message to any chat and open its preview
• tab on the bottom right three-dot menu icon
• open Playback speed -> see the same behavior

popover.mp4

So should it be resolved in this PR, or can a separate ticket be created?
cc @cristipaval

[cristipaval]

I think a separate PR should be fine

[ikevin127]

Cool, then I ✅ Approve once again!

[OSBotify]

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

[lanitochka17]

This PR is failing because of issue #43145

336992914-53fc3d1f-3341-4e76-93a5-3be9c9238390.mp4

[ikevin127]

From issue #43145:

Action Performed:

1. Open the app in mWeb
2. Go to FAB
3. Select "Send invoice"
4. Send Invoice
5. Go to user who needs to pay this invoice

Expected Result:
Verify that correct settlement button appears in preview and inside of the report header:

• Personal Card or Bank account
• Business Card or Bank account
• Forward to someone else

Actual Result:
The menu of the button presents only the option "Pay $x as an individual" and the options like the desing are not displayed.

I'm wondering:

1. Is this logic that we missed when it comes to the functionality this PR should've implemented ?
2. Is this PR a new design (approved by @Expensify/design team) and the #43145 issue's Expected result is wrong ?
3. What about the other related issues like HIGH [$750]: Clean up the payment options on Pay button in New Dot #36301 (PR #37174) and [MEDIUM] Sequential menu when paying an invoice #41976, shouldn't we align the expected result of all of these issues / PR's to match from both design and functionality POV ?

cc @VickyStash @waterim @cristipaval

[VickyStash]

@ikevin127

1. It's not the missed logic, it's an expected result to have only Pay as an individual option for now. Other options will be implemented in the future. For example, there is a separate issue for the Pay as business option implementation.
2. Yes, it was reviewed by the designer
3. I'm not sure about this one ^^ @waterim @cristipaval

[OSBotify]

🚀 Deployed to production by https://github.com/Julesssss in version: 1.4.79-11 🚀

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

[davidcardoza]

What about the other related issues like #36301 (PR #37174) and #41976, shouldn't we align the expected result of all of these issues / PR's to match from both design and functionality POV ?

Those are different pay flows. The invoice payment flow is its own isolated payment flow, separate from the IOU payment flow you referenced.