Skip to content
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

[HOLD for Payment 2024-09-06][$250] Add bank account flow has too much padding below bottom button #47569

Closed
1 of 6 tasks
m-natarajan opened this issue Aug 16, 2024 · 19 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented Aug 16, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 9.0.21-0
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @shawnborton
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1723725008039179

Action Performed:

  1. Go to workspace
  2. Add bank account
  3. Continue to step 2 personal page
  4. Observe the padding below the Next button

Expected Result:

They should just have 20px padding on the left/right/bottom

Actual Result:

All of the bottom docked buttons have too much padding below them

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

CleanShot 2024-08-15 at 08 30 14@2x
CleanShot 2024-08-15 at 08 29 32@2x
Snip - (5) New Expensify - Google Chrome

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~016d5ff6180f00c2d9
  • Upwork Job ID: 1825648516021828712
  • Last Price Increase: 2024-08-19
  • Automatic offers:
    • ikevin127 | Reviewer | 103620843
    • cretadn22 | Contributor | 103620845
Issue OwnerCurrent Issue Owner: @ikevin127
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 16, 2024
Copy link

melvin-bot bot commented Aug 16, 2024

Triggered auto assignment to @johncschuster (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@cretadn22
Copy link
Contributor

cretadn22 commented Aug 16, 2024

Edited by proposal-police: This proposal was edited at 2024-08-16 15:52:37 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Add bank account flow has too much padding below bottom button

What is the root cause of that problem?

submitButtonStyles={[styles.mb0, styles.pb5]}

We added bottom padding to this button.

What changes do you think we should make in order to solve the problem?

Let's remove the bottom padding.

To maintain consistency, we should also remove the bottom padding from the submit button on other forms.

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Aug 19, 2024
Copy link

melvin-bot bot commented Aug 19, 2024

@johncschuster Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@johncschuster johncschuster added the External Added to denote the issue can be worked on by a contributor label Aug 19, 2024
Copy link

melvin-bot bot commented Aug 19, 2024

Job added to Upwork: https://www.upwork.com/jobs/~016d5ff6180f00c2d9

@melvin-bot melvin-bot bot changed the title Add bank account flow has too much padding below bottom button [$250] Add bank account flow has too much padding below bottom button Aug 19, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 19, 2024
Copy link

melvin-bot bot commented Aug 19, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @ikevin127 (External)

@melvin-bot melvin-bot bot removed the Overdue label Aug 19, 2024
@ikevin127

This comment has been minimized.

@ikevin127
Copy link
Contributor

@cretadn22's proposal looks good to me. The RCA is correct and the proposed solution fixes the issue.

I agree that in order to have consistency between the user and workspace BA flows we should fix this in all workspace BA flow pages:

PR Notes .

For the following components we only need to remove styles.pb5:

  • Address.tsx
  • AddressBusiness.tsx
  • AddressUBO.tsx
  • BeneficialOwnerCheckUBO.tsx
  • DateOfBirth.tsx
  • DateOfBirthUBO.tsx
  • FullName.tsx
  • IncorporationDateBusiness.tsx
  • LegalNameUBO.tsx
  • NameBusiness.tsx
  • PhoneNumberBusiness.tsx
  • SocialSecurityNumber.tsx
  • SocialSecurityNumberUBO.tsx
  • TaxIdBusiness.tsx
  • WebsiteBusiness.tsx

For this one remove styles.mh0 from style and replace styles.p5 with styles.mh5 in submitButtonStyles:

  • IncorporationStateBusiness.tsx

For this one replace styles.p5 with styles.mh5 in submitButtonStyles:

  • TypeBusiness.tsx

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Aug 21, 2024

Triggered auto assignment to @dangrous, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@dangrous
Copy link
Contributor

great, that works for me - assigning! We should also have someone from @Expensify/design review the PR once it's ready to make sure we are looking good.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 21, 2024
Copy link

melvin-bot bot commented Aug 21, 2024

📣 @ikevin127 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

Copy link

melvin-bot bot commented Aug 21, 2024

📣 @cretadn22 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@shawnborton
Copy link
Contributor

Yup, please tag us in the PR when it's ready!

@ikevin127
Copy link
Contributor

I noticed that this padding does not exist on the User - Add BA flow but it exists on the Workspace - Add BA flow.

I asked on Slack 🧵thread if anyone familiar can shed some light on why this is the case and all I was told so far is that, when testing on mobile (mWeb / Native) to make sure that the Next button is accessible and not covered by device safe areas.

Given this padding was already not present on the User - Add BA flow and there are no issues there on mobile, I tend to think that we're not going to have a problem.

I'll make sure to report any potential issue during testing 👍

@cretadn22 cretadn22 mentioned this issue Aug 22, 2024
50 tasks
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 22, 2024
@cretadn22
Copy link
Contributor

@ikevin127 I just reviewed it again on staging. Please check out my video here:

Screen21.mov

It looks like the bottom padding appears on the step pages but not on the confirmation page. We probably shouldn't have padding at the bottom here. It seems like the submitButtonStyles might have been applied incorrectly during implementation.

@cretadn22
Copy link
Contributor

@ikevin127 PR is ready

@ikevin127
Copy link
Contributor

⚠️ We just got confirmation on Slack that the Deploy Checklist: New Expensify 2024-08-26 which includes the PR of this issue was only deployed to production today in Deploy Checklist: New Expensify 2024-08-28. More context on why this happened can be found in this Slack thread and this Slack comment.

Given the context above, this issue should be on [HOLD for Payment 2024-09-6] according to today’s production deploy from Deploy Checklist: New Expensify 2024-08-28.

cc @johncschuster @dangrous

@johncschuster johncschuster changed the title [$250] Add bank account flow has too much padding below bottom button [HOLD for Payment 2024-09-06][$250] Add bank account flow has too much padding below bottom button Sep 3, 2024
@johncschuster
Copy link
Contributor

Thanks, @ikevin127! I'll get payment issued one we've passed the regression threshold 👍

@ikevin127
Copy link
Contributor

Regression Test Proposal

  1. Login into the Expensify app.
  2. Create a new workspace and go to workspace settings.
  3. Add bank account.
  4. Continue to step 2 personal page.
  5. Observe that there is no extra padding below the Next button.
  6. Also verify this on all the other pages on the bank account connection flow.

Do we agree 👍 or 👎.

@johncschuster
Copy link
Contributor

Payment has been issued! Thanks for your contributions! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

6 participants