Conversation
|
@dominictb 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] |
Reviewer Checklist
Screenshots/Videos |
|
For this point in the design doc:
@joekaufmanexpensify What if I have (a) business bank account(s) but no personal one; and vice versa: (a) personal one(s) but no business account? Should we show them in their own section with corresponding title? Or must they have both types in order to show in sections?
|
|
@koko57 We have conflicts & lints BTW. |
I will fix it. I think we don't want to show any header (section) when we only have one bank account |
|
@dominictb it is strange - is it displayed for you like this with the header? Section headers should be only visible when there is a business bank account and a personal bank account. an there should be always 2 headers |
|
conflicts resolved |
|
Oh sorry for not clarifying earlier. The screenshot was manually tweak for demonstration purpose. Currently it only shows sections if we have both types of account. |
|
@dominictb aaaa ok, thanks for clarifying! If I understood it correctly we don't want to show any header when only one bank account (or several but of one type) is displayed, but waiting for confirmation from @joekaufmanexpensify |
The latter. We will only show the section headers if you have both personal and business bank account(s). If you only have personal bank accounts (one or multiple), then no section headers. Same with one or multiple business bank accounts. But as soon as you have at least one of both types, we show them. The thinking was the section headers are overkill with only one type of bank account, since the user won't need to differentiate or really have a question about what type they are. LMK if any more Q's! |
dominictb
left a comment
There was a problem hiding this comment.
Beside the above update, everything works so well.
![]()
Co-authored-by: Dominic <165644294+dominictb@users.noreply.github.com>
|
Sweet. Let's test on adhoc once you review @srikarparsi too 👍 |
|
🚧 @srikarparsi has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
Tested! It worked well. Good with me to proceed @srikarparsi 👍 |
|
Awesome merging! |
| </OfflineWithFeedback> | ||
| ); | ||
| }, | ||
| [ |
There was a problem hiding this comment.
@koko57 You missed missed dependency here.
@srikarparsi Please consider this as regression and penalise C+ accordingly.
There was a problem hiding this comment.
Hey @shubham1206agra, can you link the issue this caused please?
There was a problem hiding this comment.
There was no issue. But it caused lint failure when another PR was merged #67435.
There was a problem hiding this comment.
It looks like that is a different lint error? The one I see is
React Hook useCallback has missing dependencies: 'styles.mb1' and 'styles.mt4'. Either include them or remove the dependency array.
Is that the one you are referencing?
|
🚀 Deployed to staging by https://github.com/srikarparsi in version: 9.1.88-0 🚀
|
|
🚀 Deployed to production by https://github.com/grgia in version: 9.1.88-3 🚀
|







Explanation of Change
Fixed Issues
$ #66072
PROPOSAL: -
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
same as above
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop