-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bank account and Authorized payer buttons displayed instead of Connect bank account #40182
Conversation
@ishpaul777 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] |
I think this looks pretty good, cc @Expensify/design (you too Tom) for extra eyes on the loading spinner pattern here. |
Looks good to me as well |
Yeah, I think the loading spinner is fine. It would be great if we could minimise that time to load, but this works for sure. |
♻ Fixed conflict by removing unused type export
@trjExpensify We can involve BE for this in a separate task regarding the |
Sorry it took me a while to get to this, reviewing now |
Reviewer Checklist
Screenshots/VideosAndroid: NativeRecord_2024-05-17-16-20-18.mp4Android: mWeb ChromeRecord_2024-05-17-16-48-49.mp4iOS: Nativetrim.0E6E945D-D25B-49BD-85BB-2E8223C1DD75.MOViOS: mWeb Safaritrim.46D089D1-2583-4D37-8C3A-74128F63ADAD.MOVMacOS: Chrome / SafariBA added- (with and without internet) Screen.Recording.2024-05-17.at.1.17.46.AM-1.movNo BA added - (with and without internet) Screen.Recording.2024-05-17.at.12.25.37.AM.movMacOS: DesktopScreen.Recording.2024-05-17.at.5.00.55.PM.mov |
bug: Infinite loading when toggling the button offline. and loading time in general is too long, i have never seen this type of loading pattern (loading for everytime user toggle on) anywhere in App we generally set all necesary optimistc data to avoid this Screen.Recording.2024-04-16.at.5.26.43.PM.mov |
@ishpaul777 Good catch, I'll address the offline case of the spinner. Regarding the seemingly long |
@ishpaul777 Fixed the infinite loading while offline bug. Let me know if you find any other issues with this PR! |
Ah, interesting. @lakchote might have some insight on that! |
@ikevin127 Can you please merge main quickly |
Thinking more about the loader it just feels off pattern, can you please share a recording for the "jump" that you mentioned in PR decription and do we get this everytime. I believe the loading is dependent on user network condition more than how much backend is optimized right? so in case user network is slow it just have long loader, which is exactly we want to avoid with have optimisticData set for every request |
|
FYI i'll be OOO until tuesday and will be partially available after that until 29 April. Status for the PR: I feel the usage of loader here is not our pattern so i am not inclined to approve it, If anyone disagrees i'd love to hear a different POV, as of this comment from @ikevin127 i do agree there's a lot going on with command.
we have 2 ways to move this forward (that i could think of)
and the other one Kevin suggested #40182 (comment) |
What's going on here? We back to finish it off? |
@trjExpensify
|
The loader pattern was initially approved by @Expensify/design team here. @Expensify/design could you please check @ishpaul777's comment here regarding the infinite loader scenario? |
Yeah, I like Tom's suggestion. |
Good idea, I've triggered a build and will let you know once it's ready. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
Thank you @lakchote for build. i was not able to reproduce with ADHOC and dev on real device. I am assuming there might be some issue with API at that time. |
@Expensify/design This is ready for you guys to review 🙇 |
Nice - are there updated screenshots for us to review? |
Yes, reviewer checklist has the up to date videos for all platforms #40182 (comment) |
: translate('workflowsPage.connectBankAccount') | ||
} | ||
description={bankDisplayName} | ||
disabled={isOffline || !isPolicyAdmin} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this screen, why does one row appear to be more faded out than the other?
it seems to done quite intentionally even before this PR. that is for the case when there's no optimistic pending action but user is offline.

we can add a check !policy?.pendingFields?.reimbursementChoice && isOffline
to solve this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think we should not have it double-faded so let's definitely solve that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
- The first disabled grayed-out fade comes from the "Make or track payments" toggle when toggled while offline.
- The second one comes from Connect bank account / Bank account option which was and is disabled while offline.
- Authorized payer option is enabled while offline -> meaning the Authorized payer can be changed optimistically. This is why this one doesn't get double disabled grayed-out fade when "Make or track payments" is toggled while offline.
ishpaul777:
we can add a check !policy?.pendingFields?.reimbursementChoice && isOffline to solve this
I don't think this logic is good since it would break the logic mentioned at (2.) where Connect bank account / Bank account option was and is supposed to be disabled while offline.
shawnborton:
Okay, I think we should not have it double-faded so let's definitely solve that
To do this, instead I would use Connect bank account / Bank account option shouldGreyOutWhenDisabled
to not have the 2nd gray-out fade applied to the option when already applied from "Make or track payments" toggle.
Similarly, we have to do the same for the OfflineWithFeedback
wrapper of Authorized payer option (3.) we add:
shouldDisableOpacity={isOffline && policy?.pendingFields?.reimbursementChoice && policy?.pendingFields?.reimburser}
in order to not double gray-out the Authorized payer if already applied from "Make or track payments" toggle, when Authorized payer is changed while offline. This is how it would look in all scenarios:
- Without Bank account added:
Offline (can't add / change Connect bank account option) |
---|
![]() |
Offline (with "Make or track payments" Toggled (optimistically)) |
---|
![]() |
- With Bank account added:
Offline (can't add / change Bank account option) |
---|
![]() |
Offline (with "Make or track payments" Toggled (optimistically)) |
---|
![]() |
Offline (with "Make or track payments" Toggled + Authorized payer changed (optimistically)) |
---|
![]() |
Offline (with "Make or track payments" Toggled + Authorized payer changed (optimistically) and hover on Authorized payer) |
---|
![]() |
shouldDisableOpacity={isOffline && policy?.pendingFields?.reimbursementChoice && policy?.pendingFields?.reimburser}
^ - Writing it like this is specifically important for the below case, since we don't want to NOT gray out Authorized payer when changed offline (optimistically) in case "Make or track payments" was NOT toggled, by only writing it like:
❌ shouldDisableOpacity={policy?.pendingFields?.reimburser}
since that would NOT gray out the optimistically set Authorized payer, instead we want to gray it out, maintaining the pattern like so:
Offline (with "Make or track payments" Not Toggled + Authorized payer changed (optimistically)) |
---|
![]() |
Please let me know if we're in agreement with the changes mentioned above from a design / code POV.
Once agreed I'll push the changes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for detailed investigation @ikevin127, code wise looks good to me 👍
Yeah! we can def. do minor style adjustment. Currently both Menuitems has padding top 12px and margin-top 16px. What should be the ideal |
Can we try the second menu item with no top margin? This way it's 12px + 12px away from the item above it. |
Did this for second menu item, this are the results:
Please let me know if we're in agreement with the changes mentioned above. Once agreed I'll push the changes! cc @shawnborton |
Looks great! |
@ishpaul777 Just pushed changes from here and here. PR is ready for review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@lakchote Are we merging this soon ? I synced w/ main because it had conflicts. |
There was a merge freeze. Now that it's been removed, I'm going to merge this. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/lakchote in version: 1.4.77-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.77-11 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.77-11 🚀
|
Details
Everytime we toggle "Make or track payments" we set the
achAccount: {reimburser: reimburserEmail}
and the UI ends up looking like we have a bank account added because of this hasVBA check, when we don't actually have a bank account added yet.To fix this we changed the
hasVBA
to check when we actually have a BA by checking forbankAccountID
, additionally added theisLoadingWorkspaceReimbursement
for theSetWorkspaceReimbursement
call in order to show a loading spinner when we toggle "Make or track payments", instead of having the UI jump from the "Connect bank account" button to "Bank account" and "Authorized payer" buttons.Small refactor: added
shouldShowBankAccount
variable because there were 3 occurances of this check.Fixed Issues
$ #39947
PROPOSAL: #39947 (comment)
Tests
Offline tests
TLDR: same as Tests.
QA Steps
TLDR: same as Tests.
Additional tests from PR:
Preconditions: Having a policy with VBA and some Admins.
Happy path:
Error path:
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android.mov
Android: mWeb Chrome
android-mweb.mov
iOS: Native
ios.mov
iOS: mWeb Safari
ios-mweb.mov
MacOS: Chrome / Safari
without-ba.mov
MacOS: Desktop
desktop.mov