-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix: Remove non-dismissable error message on the Expensify card page #75672
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@bernhardoj 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] |
trjExpensify
left a comment
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.
The UX feels weird on this one. Why does an error message show as soon as you enter the page?
CC: @joekaufmanexpensify for vis
@trjExpensify I think it's the magic code send limit error |
| ...prevState, | ||
| [cardID]: error, | ||
| })); | ||
| navigateBack(); |
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.
I see that navigateBack was added from #68749. @MonilBhavsar if the user inputs a wrong magic code, they will be navigated back.
web.mp4
This is the previous behavior. The user stays on the magic code page.
web.mp4
In this PR, we want to keep the user on the magic code page and show any errors on the magic code page. Does that fine with you?
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.
Fine with me, we implemented the fix because there was no error to display on the magic code page cc @joekaufmanexpensify
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.
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.
@bernhardoj I couldn't really test this since I can't connect with an EU/UK bank account. But I noticed that the latest change in the PR was to automatically redirect to ROUTES.SETTINGS_WALLET_CARD_MISSING_DETAILS for EU/UK cards (I hardcoded this to US)
MacOS-Chrome.mp4
Since I can't use an EU/UK bank account, I couldn't confirm whether the missing personal details error message is shown correctly
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.
@MonilBhavsar do you know how can I add a EU/UK bank account?
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.
@ryntgh Btw, can you merge with main, please?
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.
Done, merged with the latest main!
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.
@ryntgh I get this error when trying to simulate the missing details error.
You can try rejecting the promise in revealVirtualCardDetails.
reject('cardPage.missingPrivateDetails');
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.
@ryntgh after thinking more about it, I think it's better to keep the previous logic if the error is 'cardPage.missingPrivateDetails' because it's not a magic code error. What do you think?
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.
@bernhardoj Yeah, I agree. Keeping the previous logic for that case seems like the better approach
I agree with @trjExpensify. Sounds like you're saying the video you uploaded for testing with the magic code send limit error is not actually representative of what the user will normally see @bernhardoj. Is that right? |
|
@joekaufmanexpensify in most cases, yes. It only happens if the user has requested too many times, and in this case, @ryntgh recorded in that state. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.mp4Android: mWeb Chromeandroid.mweb.mp4iOS: HybridAppios.mp4iOS: mWeb Safariios.mweb.mp4MacOS: Chrome / Safariweb.mp4 |
bernhardoj
left a comment
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
|
@danieldoglas ready for your review |
Explanation of Change
This PR remove non-dismissable error message on the card page and show it on the magic code page instead
Fixed Issues
$ #73134
PROPOSAL:
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Precondition:
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)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-Native.mp4
Android: mWeb Chrome
Android-mWeb.mp4
iOS: Native
iOS-Native.mp4
iOS: mWeb Safari
iOS-mWeb.mp4
MacOS: Chrome / Safari
MacOS-Chrome.mp4
MacOS: Desktop
MacOS-Desktop.mp4