[HOLD for payment 2024-03-29] Bank account - "Continue with setup" is less indented than "Start over"

kbecciv commented 6 months 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.4.53-2 Reproducible in staging?: y Reproducible in production?: y Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

User has quit bank account setup while in the middle of it.
1. Go to staging.new.expensify.com.
2. Go to Profile > Workspaces > any workspace.
3. Go to Bank account.

Expected Result:

"Continue with setup" will have the same level of indentation as "Start over".

Actual Result:

"Continue with setup" is less indented than "Start over".

Workaround:

n/a

Platforms:

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

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01c2b89887e68785d4
Upwork Job ID: 1775720355254845440
Last Price Increase: 2024-04-04

melvin-bot[bot] commented 6 months ago

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

kbecciv commented 6 months ago

@mallenexpensify I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

kbecciv commented 6 months ago

We think that this bug might be related to #wave-collect - Release 1

ghost commented 6 months ago

Proposal

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

Bank account - "Continue with setup" is less indented than "Start over"

What is the root cause of that problem?

The root cause of the problem is here : https://github.com/Expensify/App/blob/7742bec5c32e4118963fcdc67d3c15b7412f3838/src/styles/index.ts#L3492-L3498 we have more margin to the left for "Start Over" text in comparison to "Continue"

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

We need to reduce it from 8 to 7. This should fix it

What alternative solutions did you explore? (Optional)

N/A

Result

shahinyan11 commented 6 months ago

Proposal

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

Bank account - "Continue with setup" is less indented than "Start over"

What is the root cause of that problem?

There is not set iconStyles prop on Button component as this is done for the connect online with Plaid button here

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

Add iconStyles={[styles.customMarginButtonWithMenuItem]} prop on Button

What alternative solutions did you explore? (Optional)

ShridharGoel commented 6 months ago

Proposal

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

Bank account - "Continue with setup" is less indented than "Start over".

What is the root cause of that problem?

Continue with setup uses Button while Start over uses MenuItem. https://github.com/Expensify/App/blob/7742bec5c32e4118963fcdc67d3c15b7412f3838/src/pages/ReimbursementAccount/ContinueBankAccountSetup.js#L67-L84

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

Use Button instead of MenuItem

<Button
    text={props.translate('workspace.bankAccount.startOver')}
    icon={Expensicons.RotateLeft}
    onPress={() => BankAccounts.requestResetFreePlanBankAccount()}
    style={[styles.mv0]}
    shouldShowRightIcon
    large
    disabled={Boolean(pendingAction) || !_.isEmpty(errors)}
/>

ShridharGoel commented 6 months ago

Proposal

Updated style and added screenshot.

shahinyan11 commented 6 months ago

Proposal

Updated. Added a link to a similar implementation like mine in another place

abzokhattab commented 6 months ago

Proposal

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

Connect with setup button styles are not the same as the Connect online with plaid

What is the root cause of that problem?

Connect with setup button styles here

https://github.com/Expensify/App/blob/c9f88619cceb919ed748f97ff57d2168bb871e8d/src/pages/ReimbursementAccount/ContinueBankAccountSetup.js#L67-L76

are not the same as in the Connect online with plaid: https://github.com/Expensify/App/blob/85bf1ece78cedd9c6ba95217032cb956cfdf4acd/src/pages/ReimbursementAccount/BankAccountStep.js#L136-L152

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

we should add to the Connect with setup button component to preserve styles consistency between the two buttons

                            iconStyles={[styles.customMarginButtonWithMenuItem]}
                            innerStyles={[styles.pr2, styles.pl4, styles.h13]}

melvin-bot[bot] commented 6 months ago

Triggered auto assignment to @dannymcclain (Design), see these Stack Overflow questions for more details.

mallenexpensify commented 6 months ago

@dannymcclain , 👀 on the screenshot in the OP. Do you think it's worth fixing? If so, I'll start the price at $250 since it seems like a relatively easy bug.

dannymcclain commented 6 months ago

@mallenexpensify I do think it's worth fixing. But @shawnborton, didn't we just run into this issue on another PR? Was it the one about button styles? Maybe this should just be a follow up to that if it's the same problem.

ghost commented 6 months ago

@dannymcclain This is similar issue like this https://github.com/Expensify/App/issues/38291

dannymcclain commented 6 months ago

That's the one! Thank you @godofoutcasts94

ShridharGoel commented 5 months ago

@dannymcclain To maintain UI consistency, shouldn't we use Button component for both the options? Currently, one of them uses Button and the other uses MenuItem because of which they look different from each other. We can have both as Buttons, with different background colours. You can check the screenshot posted here.

shawnborton commented 5 months ago

Actually yeah, I think @narefyev91 already had a fix for this? Can you confirm?

narefyev91 commented 5 months ago

Actually yeah, I think @narefyev91 already had a fix for this? Can you confirm?

Yup fix was done for some other places - i can add one more for this one

shawnborton commented 5 months ago

Okay great, that would be good. Just flagging @luacmartins @mountiny for visibility as this would be another #ideal-nav follow up.

melvin-bot[bot] commented 5 months ago

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

melvin-bot[bot] commented 5 months ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 5 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.55-3 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

https://github.com/Expensify/App/pull/38608

If no regressions arise, payment will be issued on 2024-03-29. :confetti_ball:

melvin-bot[bot] commented 5 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:

[x] [@jasperhuangg] The PR that introduced the bug has been identified. Link to the PR: This seems to be a new feature
[x] [@jasperhuangg] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A
[x] [@jasperhuangg] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
[x] [@jasperhuangg] Determine if we should create a regression test for this bug. Not necessary–it's a minor display bug that shouldn't crop up again.
[x] [@jasperhuangg] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[x] [@mallenexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

dannymcclain commented 5 months ago

Any news here?

mallenexpensify commented 5 months ago

@Pujan92 , you reviewed the PR so you're due compensation here, right?

Assuming so, can you complete the above BZ checklist? I imagine we either don't need a testrail GH or it'd be one we just check everyone month as part of the design and edge case test run.

Pujan92 commented 5 months ago

@Pujan92 , you reviewed the PR so you're due compensation here, right?

Yes @mallenexpensify

Assuming so, can you complete the above BZ checklist?

It is already completed by @jasperhuangg.

melvin-bot[bot] commented 5 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01c2b89887e68785d4

melvin-bot[bot] commented 5 months ago

Current assignee @Pujan92 is eligible for the Internal assigner, not assigning anyone new.

mallenexpensify commented 5 months ago

@Pujan92 can you please accept the job and reply here once you have? https://www.upwork.com/jobs/~01c2b89887e68785d4

Pujan92 commented 5 months ago

@Pujan92 can you please accept the job and reply here once you have? https://www.upwork.com/jobs/~01c2b89887e68785d4

Done..

mallenexpensify commented 5 months ago

Contributor+: @Pujan92 paid $500 via Upwork.

Thanks

Expensify / App