-
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 verification modal info #49536
Bank account verification modal info #49536
Conversation
src/pages/workspace/expensifyCard/WorkspaceExpensifyCardListPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/workspace/expensifyCard/WorkspaceExpensifyCardListPage.tsx
Outdated
Show resolved
Hide resolved
Shared the instructions for how to create the verifying bank account with you in our dm |
@dubielzyk-expensify @DylanDylann 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] |
This is my bad (as I did the mocks), but I wonder when I see it now if we should reduce the height of the illustration/video above "Verification in progress". Our empty states have 220px height, but that feels too small though I see that in Figma @shawnborton commented on using the empty state component here and change the illustration like so: Keen to hear if @Expensify/design agrees. |
@dubielzyk-expensify ok, so please confirm if I should make these changes |
I would love to stay consistent with other empty states. I think they are all 220px in height, right? |
Yep. 220px.
…On Mon, Oct 28, 2024 at 6:41 PM Shawn Borton ***@***.***> wrote:
I would love to stay consistent with other empty states. I think they are
all 220px in height, right?
—
Reply to this email directly, view it on GitHub
<#49536 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/BCVOE6YDEZGV6GRO2QEEIJTZ5XTDTAVCNFSM6AAAAABOSCLNY6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBQHA4DKMJSG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@dubielzyk-expensify @shawnborton ok, so for the Verification state we're changing the illustration for this one: Should the height of the illustration also change to 220px for the empty card state modal? |
Might have to adjust, but the container should be I think we have the illustration already checked in too |
I don't think we want to change the illustration personally, I think if the verification is in process, we want to use what Jon has here. cc @Expensify/design for more thoughts though! |
ok, so I will change only the Verification Modal image for now. If you decide to do some changes to the Empty State modal just ping me! 😊 |
Nice. And does that illustration already exist in the repo, or do you need us to export any assets? |
Totally agree Shawn! |
Lovely! |
Looks great! |
@DylanDylann ready for review |
I will try to complete my reviewing for these issues on Wed |
Reviewer Checklist
Screenshots/Videos |
Discover new bug (not relate to this PR): https://expensify.slack.com/archives/C05LX9D6E07/p1730270492636799?thread_ts=1728966082.429659&cid=C05LX9D6E07 |
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!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.56-0 🚀
|
I dont think this performance regression could be legit on this pr |
🚀 Deployed to production by https://github.com/Julesssss in version: 9.0.56-9 🚀
|
Details
Fixed Issues
$ #49472
PROPOSAL:
Tests
(if you have already a workspace with Expensify Card enabled and you can issue a new card there, you can skip steps 1-4)
Offline tests
QA Steps
(if you have already a workspace with Expensify Card enabled and you can issue a new card there, you can skip steps 1-4)
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label 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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop