[HOLD for payment 2023-10-05] [$500] IOS - Inconsistent margin above Next button in 2FA Step 2

[kbecciv]

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

---

Action Performed:

1. Go to settings-> Click Security
2. Click Two-factor authentication-> Click Copy
3. Click Next

Expected Result:

Margin should be consistent

Actual Result:

Inconsistent margin above Next button on 2FA Step 2 screen

Workaround:

Unknown

Platforms:

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

• [ ] Android / native
• [ ] Android / Chrome
• [x] iOS / native
• [ ] iOS / Safari
• [ ] MacOS / Chrome / Safari
• [ ] MacOS / Desktop

Version Number: 1.3.53.2 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 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/93399543/3d3e7775-2e85-4f10-9bfe-47cde3cd4680

Expensify/Expensify Issue URL: Issue reported by: @Nathan-Mulugeta Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1684403210538459

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c6a99b3ba9660c1d
• Upwork Job ID: 1699603208696877056
• Last Price Increase: 2023-09-14
• Automatic offers:
• neonbhai | Contributor | 26761577
• Nathan-Mulugeta | Reporter | 26761583

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Bug0 Triage Checklist (Main S/O)

• [ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• [ ] This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• [ ] This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• [ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[neonbhai]

Proposal

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

Inconsistent margin above Next button in 2FA Step 2

What is the root cause of that problem?

The ScrollView on this page has a marginBottom of 20px (styles.mb5) added here: https://github.com/Expensify/App/blob/17025aa62b1a8c40bb451a97000d3f4605c7bdae/src/pages/settings/Security/TwoFactorAuth/Steps/VerifyStep.js#L78-L79

This is responsible for the extra gap on this page of the flow.

Note The other pages follow the standard 8px gap on top of the footer button.

Update:

The root cause for the button appearing over the content for small screens is because the Footer button is placed outside the ScrollView here which separates its rendering logic from the content (and makes it render over the content)

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

• Fix padding of button:

  • We should remove this margin to be consistent with other pages in the flow. This would mean removing the style prop from ScrollView here: https://github.com/Expensify/App/blob/17025aa62b1a8c40bb451a97000d3f4605c7bdae/src/pages/settings/Security/TwoFactorAuth/Steps/VerifyStep.js#L79

Update:

• Make button scrollable below content:

  • To accomplish this we will make the FixedFooter component here (which includes the button) as part of the Parent ScrollView here: https://github.com/Expensify/App/blob/fc8adf99a11a41154ec276d44495485958e43a7d/src/pages/settings/Security/TwoFactorAuth/Steps/VerifyStep.js#L78-L81
  • We will also add contentContainerStyle={styles.flexGrow1} as ScrollView style here to make sure the content takes all available space
  • Increase padding for the FixedFooter component so the button has breathing space: https://github.com/Expensify/App/blob/fc8adf99a11a41154ec276d44495485958e43a7d/src/pages/settings/Security/TwoFactorAuth/Steps/VerifyStep.js#L114 This will make the button sit below the content (and be scrollable, but also sit on the bottom of the modal if container size is bigger than content)
  • We do not need another component for the button wrapper. FixedFooter sits at the bottom of the view it is rendered in, so it fits our usecase. FixedFooter is also part of a view of our BaseSelectionList core component here. An alternative would be to add a simple View wrapper to the button that will make it sit at the bottom of the view.
  • We should do the same for CodesStep.js page in the flow, where we move the Button here inside the Parent ScrollView here

Result (VerifyStep.js)

### Final Result **The button is scrollable after the content but stays fixed at the footer if contentsize is less then the available space:** [TwoFactorAuth.webm](https://github.com/Expensify/App/assets/64629613/5988ad83-8bfc-429a-8975-14fb6b5f94ac)

Result (CodesStep.js)

### Final Result **The button is scrollable after the content but stays fixed at the footer if contentsize is less then the available space:** [codes-step scroll changes.webm](https://github.com/Expensify/App/assets/64629613/8d235592-c99f-4891-af59-e5fdc631374d)

What alternative solutions did you explore? (Optional)

• We can remove padding from all steps, by incorporating the above change and also removing the styles.pt2 from the fixed footers

  • here: https://github.com/Expensify/App/blob/17025aa62b1a8c40bb451a97000d3f4605c7bdae/src/pages/settings/Security/TwoFactorAuth/Steps/VerifyStep.js#L114-L115

  • And here: https://github.com/Expensify/App/blob/17025aa62b1a8c40bb451a97000d3f4605c7bdae/src/pages/settings/Security/TwoFactorAuth/Steps/CodesStep.js#L108-L109

• We can add some breathing room on the workspace list footer button by adding consistent styes.pt2top padding on the button here: https://github.com/Expensify/App/blob/3f8f2c43430427cf3cf323fbf16013df72e59ff4/src/pages/workspace/WorkspacesListPage.js#L192-L198

  • To accomplish this, we fill be wrapping the button a with style prop of styles.pt2 in the WorkspacesListPage.js file:

  footer={
      <View style={[styles.pt2]}> 
           <Button 
               accessibilityLabel={translate('workspace.new.newWorkspace')} 
               success 
               text={translate('workspace.new.newWorkspace')} 
               onPress={() => App.createWorkspaceAndNavigateToIt('', false, '', false, !isSmallScreenWidth)} 
           /> 
       </View>
   }

[hoangzinh]

Proposal

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

IOS - Inconsistent margin above Next button in 2FA Step 2 + the submit button is fixed in the bottom of screen view instead of the page content.

What is the root cause of that problem?

Inconsistent margin above Next button in 2FA Step 2

There are 2 places that make there a lot of spacing above Next button

1. There are some marginTop and paddingTop for the button in the verify page https://github.com/Expensify/App/blob/3f8f2c43430427cf3cf323fbf16013df72e59ff4/src/pages/settings/Security/TwoFactorAuth/Steps/VerifyStep.js#L114

2. There is a margin bottom from ScrollView here https://github.com/Expensify/App/blob/3f8f2c43430427cf3cf323fbf16013df72e59ff4/src/pages/settings/Security/TwoFactorAuth/Steps/VerifyStep.js#L79

The submit button is fixed in bottom of the screen view instead of the page content We're putting the submit button outside of ScrollView, and we're using FixedFooter component as a wrapper https://github.com/Expensify/App/blob/b11bddc054f54bb46d5fb5aaf05e25b8a7df7faf/src/pages/settings/Security/TwoFactorAuth/Steps/VerifyStep.js#L114-L126

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

1. We need to move submit button inside the ScrollView component, after the Form view here
2. We need to replace FixedFooter by another component (because it's not fixed footer anymore). We can introduce another component, I.e ScreenSubmitButtonWrapper, almost same as FixedFooter component, but with default style as style={[styles.mt5, styles.mh5, styles.flex1, styles.justifyContentEnd]}. It's almost align with FormAlertWithSubmitButton
3. We need to add props contentContainerStyle={styles.flexGrow1} to ScrollView component here so it will expand to take all remaining space of screen.

We can apply above changes for other steps of 2FA.

Result:

https://github.com/Expensify/App/assets/9639873/5b11f7fd-50fd-430b-8c36-980f5b7359d7

[melvin-bot[bot]]

@JmillsExpensify Eep! 4 days overdue now. Issues have feelings too...

[JmillsExpensify]

Good one. I agree we should fix the margins and ensure they're consistent. cc @Expensify/design to make sure the spec if clear.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Current assignee @JmillsExpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot[bot]]

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

[shawnborton]

Hmm I am not entirely sure what's being proposed here, but I think we just need to get rid of the additional padding above the green button, does that sound right?

I also thought we had a pattern where if the page content is longer than the available viewport, the green button would sit directly below the last bit of page content and not cover it up. For example, take a look at the Display name page, which has the ideal behavior I think:

Also, it looks like this effects other pages too:

[mananjadhav]

@shawnborton do we then want to cover the floating button in this issue's scope? or we create another issue for that and only fix the padding here?

[shawnborton]

They are not separate issues IMO. If we fix the button behavior to match the contact methods page, the padding should be fixed at the same time.

[neonbhai]

Proposal

Updated

[hoangzinh]

Proposal updated https://github.com/Expensify/App/issues/26212#issuecomment-1697750461

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@JmillsExpensify, @mananjadhav Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot[bot]]

@JmillsExpensify @mananjadhav this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[JmillsExpensify]

Agree with @shawnborton on the solution and next steps.

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[mananjadhav]

@neonbhai's proposal here looks good to me. Apologies for the delay, I was OOO for past few days.

🎀 👀 🎀 C+ reviewed.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@JmillsExpensify @mananjadhav @bondydaa this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[melvin-bot[bot]]

📣 @mananjadhav Please request via NewDot manual requests for the Reviewer role ($500)

[melvin-bot[bot]]

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

[melvin-bot[bot]]

📣 @Nathan-Mulugeta 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[bondydaa]

waiting on PR

[neonbhai]

@mananjadhav Hi, The PR is up!

[neonbhai]

@marcochavezf PR all done, waiting for your review!

[melvin-bot[bot]]

🎯 ⚡️ Woah @mananjadhav / @neonbhai, great job pushing this forwards! ⚡️

The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉

• when @neonbhai got assigned: 2023-09-19 17:30:04 Z
• when the PR got merged: 2023-09-22 15:11:18 UTC

On to the next one 🚀

[melvin-bot[bot]]

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

[melvin-bot[bot]]

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.74-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/27885

If no regressions arise, payment will be issued on 2023-10-05. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• [ ] External issue reporter
• [ ] Contributor that fixed the issue
• [ ] Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @mananjadhav does not require payment (Eligible for Manual Requests)
• @neonbhai requires payment offer (Contributor)
• @Nathan-Mulugeta requires payment offer (Reporter)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot[bot]]

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:

• [ ] [@mananjadhav] The PR that introduced the bug has been identified. Link to the PR:
• [ ] [@mananjadhav] 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:
• [ ] [@mananjadhav] 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:
• [ ] [@mananjadhav] Determine if we should create a regression test for this bug.
• [ ] [@mananjadhav] 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.
• [ ] [@JmillsExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[mananjadhav]

We added the FixedFooter to the page in this PR. This is where we could've tested the positioning of the button.

I don't think we need any change in the process, as this as a bug. I also don't think we need to add regression test here.

[neonbhai]

Friendly bump for payment^

cc: @JmillsExpensify

[JmillsExpensify]

Payment summary:

• Issue reporter: @Nathan-Mulugeta $250
• Contributor: @neonbhai $1,000 (incl. urgency bonus)
• Contributor+: @mananjadhav $1,000 (incl. urgency bonus)

[JmillsExpensify]

$1,000 payment approved for @mananjadhav based on above summary.

[JmillsExpensify]

Issue reporter and contributor are paid out, no regression tests. Closing this issue.

[Nathan-Mulugeta]

Hey @JmillsExpensify this issue was created on Aug 29, prior to bounty adjustment announcement, the reporter bounty should be $250 not $50, thanks.

[neonbhai]

That is true, this should adhere to the earlier pay scale (for contributors also I hope :bow: )

cc: @JmillsExpensify

[mananjadhav]

@JmillsExpensify Can you help with the previous questions from the contributors? I am not sure what's the process for the old bounty?

Also I don't see the request being approved on the NewDot.

Lastly @JmillsExpensify I am a bit confused how the amount is $1000. If we consider the old bounty it should be $1500, and if we consider the new bounty it should be $750.

[flaviadefaria]

@JmillsExpensify following this comment Nathan-Mulugeta needs to be paid an extra $200 since the bug was reported under the old bounty.

@Nathan-Mulugeta I just sent you an offer for the remaining $200 please accept it so that I can pay you.

[Nathan-Mulugeta]

Thank you so much @flaviadefaria , I just accepted the offer.

[flaviadefaria]

Paid, thanks for your patience!

[JmillsExpensify]

@mananjadhav yes it should be $750. I got confused working through that. Given that this was my mistake, I'll honor that amount, which benefits you.

[JmillsExpensify]

@neonbhai Your proposal was made on Aug 29th, and your proposal wasn't accepted until later. Given that both those dates come after 25 August, the old rates do not apply.

[JmillsExpensify]

In contrast, I agree with @Nathan-Mulugeta that his report came before the new payments were establish – in fact, if you look at the original report it was months ago – he is eligible for the older amounts.

[neonbhai]

Thanks for clarifying @JmillsExpensify!