-
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
Center loading indicator #10823
Center loading indicator #10823
Conversation
Can you handle this also! |
Yes sure, please suggest the page for adding a bank account via workspace. I am not getting an option to add a bank account in my workspace. |
Find usages of AddPlaidBankAccount @varshamb |
@Santhosh-Sellavel Done. |
@varshamb Do you mind updating the screenshots to include both the updated views? Thanks! |
@jasperhuangg Done. |
@varshamb |
@jasperhuangg updated! |
@varshamb Still web & desktop videos are not updated, can you update them properly? |
Take this PR as an example, nothing from the app screen is cut off. |
@Santhosh-Sellavel Is it really needed to update those screen shots, we used cc: @jasperhuangg |
Actually, that's what @jasperhuangg has asked here to update.
At least the RHN should be completely visible to ensure it's working fine on both platforms! |
@Santhosh-Sellavel updated. |
|
Thanks for updating, mostly looks good just a couple more things. Can you update the test steps for workspace flow too? Workspace FlowSettings -> Workspace -> Connect Bank Account Should be added in both |
@Santhosh-Sellavel Thanks for your inputs. |
Hey @varshamb thanks for updating those screenshots! I know it's tedious but it's important that we have a correct log of everything that was fixed in the PR. I also updated your testing steps to include a third step:
It's important that you list specifically what to check, this saves time for the QAer since they don't need to read through the issue/PR description to get context. Overall looks good to me I am approving 👍 |
@jasperhuangg LGTM |
@jasperhuangg looks like this was merged without passing tests. Please add a note explaining why this was done and remove the |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Cherry-picked to staging by @francoisl in version: 1.2.0-3 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. Note: we had a conflict on the |
🚀 Deployed to staging by @jasperhuangg in version: 1.2.1-0 🚀
|
Details
Loading indicator was not in the center of container on page
addPersonalBankAccountPage
andBankAccountStep
.We made it center using
styles.flex1
.Fixed Issues
$ #10362
Tests
Steps for adding Payment method in addPersonalBankAccountPage:
Steps for BankAccountStep in Workplace:
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)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)QA Steps
Steps for adding Payment method in addPersonalBankAccountPage:
Steps for BankAccountStep in Workplace:
Screenshots
Web
addPersonalBankAccountPage:
BankAccountStep:
Mobile Web
addPersonalBankAccountPage:
WhatsApp.Video.2022-09-05.at.23.05.27.mp4
BankAccountStep:
WhatsApp.Video.2022-09-08.at.22.43.33.mp4
Desktop
addPersonalBankAccountPage:
BankAccountStep:
iOS
addPersonalBankAccountPage:
WhatsApp.Video.2022-09-05.at.23.08.37.mp4
BankAccountStep:
WhatsApp.Video.2022-09-07.at.22.32.33.mp4
Android
addPersonalBankAccountPage:
BankAccountStep: