[HOLD for payment 2024-03-11] [$500] Login - Two offline indicators in login screen

[kbecciv]

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: v1.4.44-3
Reproducible in staging?: y
Reproducible in production?: n
Reporter: Applause - Internal Team

Action Performed:

1. Launch New Expensify app.
2. Log out if logged in.
3. Go offline.

Expected Result:

There will be only one offline indicator.

Actual Result:

There are two offline indicators - One below email input, one at the bottom of the screen.

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

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c29d8f94d59d6b6e
• Upwork Job ID: 1762550251289456640
• Last Price Increase: 2024-02-27

[melvin-bot]

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

[melvin-bot]

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

[github-actions]

👋 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]

Triggered auto assignment to @marcaaron (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[kbecciv]

We think that this bug might be related to #vip-vsb
@quinthar

[dukenv0307]

Proposal

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

• Login - Two offline indicators in login screen

What is the root cause of that problem?

• Regression from PR
• We have 2 offline indicators in here

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

• Just need to remove one <OfflineIndicator style={[styles.m0, styles.pl0, styles.alignItemsStart]} /> in SiginInPageContent

What alternative solutions did you explore? (Optional)

• Also, we can set shouldShowOfflineIndicator: false in ScreenWrapper in SiginPage. But I like the UI in the main solution

[gijoe0295]

Proposal

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

Two offline indicators in login screen

What is the root cause of that problem?

We have two OfflineIndicators in ScreenWrapper and SignInPageContent where ScreenWrapper is wrapping the other.

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

Pass shouldShowOfflineIndicator=false to ScreenWrapper in SignInPage to avoid the bottom indicator. Because previously, we have always shown the offline indicator right under the login form.

What alternative solutions did you explore? (Optional)

Remove the indicator in SignInPageContent to remove the one right under the login form.

[neonbhai]

Proposal

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

Two offline indicators in login screen

What is the root cause of that problem?

We have redundant offline indicators for login flows rendered here and here

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

We should not show the offline indicator here for small screens (!isSmallScreenWidth):

App/src/pages/signin/SignInPageLayout/SignInPageContent.tsx

Lines 55 to 57 in d2a9c6f

	<View style={[styles.mb8, styles.signInPageWelcomeTextContainer, styles.alignSelfCenter]}>
	<OfflineIndicator style={[styles.m0, styles.pl0, styles.alignItemsStart]} />
	</View>

{!isSmallScreenWidth && 
    <View style={[styles.mb8, styles.signInPageWelcomeTextContainer, styles.alignSelfCenter]}>
        <OfflineIndicator style={[styles.m0, styles.pl0, styles.alignItemsStart]} />
    </View>
}

Result

This looks good (at the bottom for mobile devices and below form for widescreen layout)

32cca55b-678c-490b-a764-aa5d2af51afc.mp4

[allroundexperts]

@marcaaron Can we clarify on where to show the indicator on large and small screens?

In Prod, its always below the form for both large and small screens FYI.

If we want to maintain the behaviour on Prod, then @gijoe0295's proposal looks good.

If however, we want to show the indicator at the bottom of the page for both large and small screens, then @dukenv0307's proposal looks good.

Lastly, if we want a mix and match of both (on large screens display the indicator at the bottom of form and on small screen display it at the bottom of the page), then @neonbhai's proposal would work well!

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[neonbhai]

Maybe we could have the design team take a look

cc: @shawnborton @dubielzyk-expensify for visibility!

[dukenv0307]

@allroundexperts @marcaaron Also note that this is deploy blocker, so we need to find out the PR that caused the regression (that is only mentioned in my proposal). A similar case here

[allroundexperts]

@dukenv0307 The mentioned PR is within the regression period so the authors should fix this.

[dukenv0307]

Yes. It is up to C+ and internal team to decide who should fix the PR.

[marcaaron]

Thanks so much for the help here. We have decided this is not urgent enough to block the deploy so let's assign it back to whoever created the regression. Which I believe would be @suneox in this case? @allroundexperts (or anyone) please kindly confirm that this PR did indeed cause the regression? 🙇

[dubielzyk-expensify]

I like what this proposal is showing, but I'm also not convinced we need to have it at the bottom for smaller screens. I could see the offline indicator working just underneath the button for all screen sizes. That way it's very clear and obvious. @Expensify/design for thoughts

[suneox]

Thanks so much for the help here. We have decided this is not urgent enough to block the deploy so let's assign it back to whoever created the regression. Which I believe would be @suneox in this case? @allroundexperts (or anyone) please kindly confirm that this PR did indeed cause the regression? 🙇

I'd like to confirm this PR raise regression, I'll handle at this PR

[shawnborton]

@dubielzyk-expensify makes a fair point - we could just pick one spot and use it across both mobile and desktop.

[dannymcclain]

Agree with the other designers—I kinda think we should just stick with just below the button for all viewport sizes. Especially because the button is disabled. I think keeping the indicator close would be helpful/make offline status (and therefore why you can't sign in) more obvious.

[marcaaron]

Many thanks @suneox 🙇

[marcaaron]

Looks like the fix is on staging.

[melvin-bot]

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

• remove dupplicate offline Indicator #37385

If no regressions arise, payment will be issued on 2024-03-11. 🎊

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

• @allroundexperts requires payment through NewDot Manual Requests

[melvin-bot]

Issue is ready for payment but no BZ is assigned. @laurenreidexpensify you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

[melvin-bot]

Payment Summary

Upwork Job

• Reviewer: @allroundexperts owed $500 via NewDot

BugZero Checklist (@laurenreidexpensify)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1762550251289456640/hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[laurenreidexpensify]

Payment Summary here

[JmillsExpensify]

$500 approved for @allroundexperts