search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.53k stars 2.88k forks source link

[$500] Bank account - Extra padding between bank account selector and ToS checkbox #30588

Closed lanitochka17 closed 11 months ago

lanitochka17 commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 1.3.93-0 Reproducible in staging?:Y Reproducible in production?: Cannot check bank account on production 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: Applause - Internal Team Slack conversation:

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to Settings > Workspaces > any workspace > Bank account > Connect with Plaid
  3. Proceed with adding Chase bank account
  4. Select a bank account in the selection page

Expected Result:

There is no extra padding between bank account selector and ToS checkbox in bank account selection page

Actual Result:

There is extra padding between bank account selector and ToS checkbox in bank account selection page

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari https://github.com/Expensify/App/assets/78819774/73c45a6a-24f1-42b0-8427-64d71bc02c7e Bug6256876_1698677897557!Screenshot_2023-10-30_at_21 42 34
MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~015f42c68e154b464f
  • Upwork Job ID: 1719030382399614976
  • Last Price Increase: 2023-10-30
  • Automatic offers:
    • ishpaul777 | Contributor | 27459210
melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @laurenreidexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

OSBotify commented 1 year ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @francoisl (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

ishpaul777 commented 1 year ago

Proposal

Problem

Extra padding between bank account selector and ToS checkbox

Root cause

We have added margin Bottom at bankAccount selector here and also margin top here on CheckboxWithLabel resulting in extra gap between the components.

Changes

We can either remove margin bottom style from here or remove margin top here on CheckboxWithLabel..(depends on how much gap we need 20px or 16 px)

francoisl commented 1 year ago

Thanks for looking into this @ishpaul777. It doesn't seem like any of that code changed recently, this might not be a regression from a recent PR?

It's a minor style issue that doesn't break anything, I'm going to remove the deploy blocker label so we can treat this as a regular bug.

melvin-bot[bot] commented 1 year ago

Current assignee @laurenreidexpensify is eligible for the Bug assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~015f42c68e154b464f

melvin-bot[bot] commented 1 year ago

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

saranshbalyan-1234 commented 1 year ago

Proposal

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

Bank account - Extra padding between bank account selector and ToS checkbox

What is the root cause of that problem?

We have an extra margin here https://github.com/Expensify/App/blob/5d45f0f6c86e97d4ae58ac167b6c162a610b817c/src/components/AddPlaidBankAccount.js#L242

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

We cannot remove style from CheckboxWithLabel component as this similar is being used across other connect bank pages as well, for ex https://github.com/Expensify/App/blob/5d45f0f6c86e97d4ae58ac167b6c162a610b817c/src/pages/ReimbursementAccount/BankAccountPlaidStep.js#L126 https://github.com/Expensify/App/blob/5d45f0f6c86e97d4ae58ac167b6c162a610b817c/src/pages/ReimbursementAccount/BankAccountManualStep.js#L127

We need to remove this style from here to get the correct fix https://github.com/Expensify/App/blob/5d45f0f6c86e97d4ae58ac167b6c162a610b817c/src/components/AddPlaidBankAccount.js#L242 Solution would look something like this

<View>
                <Picker
                    label={translate('addPersonalBankAccountPage.chooseAccountLabel')}
                    onInputChange={onSelect}
                    items={options}
                    placeholder={{
                        value: '',
                        label: translate('bankAccount.chooseAnAccount'),
                    }}
                    value={selectedPlaidAccountID}
                />
            </View>

What alternative solutions did you explore? (Optional)

N/A

narefyev91 commented 1 year ago

Proposal from @ishpaul777 looks good to me https://github.com/Expensify/App/issues/30588#issuecomment-1785535903 let's just remove bottom margin 🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 year ago

Current assignee @francoisl is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

📣 @ishpaul777 🎉 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 📖

ishpaul777 commented 1 year ago

Hii @francoisl I just have one question how do we run Plaid in sandbox mode in dev enviornment. Testing changes will require to have us bank account added. Is it possible?

francoisl commented 1 year ago

Yeah are you able to start the start the Plaid flow? If you use the staging server it should use their sandbox mode. You can choose to connect to the bank Wells Fargo, then it will show the login page for a dummy bank called "First Platypus Bank", where you can use the credentials user_good and pass_good, and skip all the other fields.

ishpaul777 commented 1 year ago

Thank you @francoisl, given steps works well for me.

ishpaul777 commented 11 months ago

@laurenreidexpensify The PR is deployed to Production last week! Looks like automation tracking the PR failed. Can payment be processed please?

laurenreidexpensify commented 11 months ago

Working on payment now

laurenreidexpensify commented 11 months ago

Payment Summary:

laurenreidexpensify commented 11 months ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

ishpaul777 commented 11 months ago

I think c+ handles checklist, contributors dont have guidelines for completing the checklist.

cc- @narefyev91

narefyev91 commented 11 months ago