[HOLD for payment 2023-11-30] [$500] Web - Animation is not visible on desktop but visible on Mobile in sign in Modal

[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: 1.3.90.1
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: @ishpaul777
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1698067583812339

Action Performed:

1. Sign out of app and open a public room - https://staging.new.expensify.com/r/7735299187836520
2. Click Sign in Button Obsereve there is no animation.
3. Open public room on mobile screen Observe there is animation visible

Expected Result:

Animation should be visible on Desktop screen also

Actual Result:

Animation not visible on Desktop 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

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

Recording.5130.mp4

MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01262777214760530e
• Upwork Job ID: 1716804131884302336
• Last Price Increase: 2023-10-31
• Automatic offers:
• tienifr | Contributor | 27550623
• ishpaul777 | Reporter | 27550625

[melvin-bot]

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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01262777214760530e

[melvin-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

[melvin-bot]

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

[kbecciv]

Proposal by: @ishpaul777
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1698067583812339

Proposal

Problem

Animation is not visible on desktop but visible on Mobile in sign in Modal

Root Cause

in [SignInPageContent] (https://github.com/Expensify/App/blob/5005161e0a4cbb0fa9e6afb693fa0690cac994c7/src/pages/signin/SignInPageLayout/SignInPageContent.js), We are showing the animation for smallscreen, This is because the same component is used for signInpage and we have different layout for the desktop in signInpage that is why the Animation only visible on smallscreen and not on desktop.

Changes

1. We can have isInModal prop from parent component SignInPageLayout passed to SignInPageContent and show the animation when the SignInPageContent used in Modal.

``jsx
{props.isSmallScreenWidth || props.isInModal ? (



) : null}

2. We also need to pass the `isInModal` to `SignInHeroImage` to set the dimensions of lottie animation.
```jsx
// src/pages/signin/SignInHeroImage.js
if (props.isSmallScreenWidth || props.isInModal) {
        imageSize = {
            height: variables.signInHeroImageMobileHeight,
            width: variables.signInHeroImageMobileWidth,
        };
    }

[ishpaul777]

Proposal

Problem

Animation is not visible on desktop but visible on Mobile in sign in Modal

Root Cause

in SignInPageContent, We are showing the animation for smallscreen, This is because the same component is used for signInpage and we have different layout for the desktop in signInpage that is why the Animation only visible on smallscreen and not on desktop.

Changes

We can have isInModal prop from parent component SignInPageLayout passed to SignInPageContent and show the animation when the SignInPageContent used in Modal.

{props.isSmallScreenWidth || props.isInModal ? (
<View style={[styles.mt8]}>
    <SignInHeroImage />
</View>
) : null}

2. We also need to pass the isInModal to SignInHeroImage to set the dimensions of lottie animation.

// src/pages/signin/SignInHeroImage.js
if (props.isSmallScreenWidth || props.isInModal) {
        imageSize = {
            height: variables.signInHeroImageMobileHeight,
            width: variables.signInHeroImageMobileWidth,
        };
    }

[tienifr]

Proposal

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

1. Animation not visible on Desktop screen.
2. There's another issue that I notice while investigating: When resizing the screen from Desktop to Mobile view, there's a significant lag in the sign ui changes, the UI stays as mobile UI for some time, then changes to the Desktop UI, see the video below:

What is the root cause of that problem?

1. The hero image currently only shows in small screen, it's missing the case where it's inside the RHN on Desktop
2. We're using the withWindowDimensions to render the components in the SignIn page. That HOC has the issue where the dimensions is only updated after a 300ms debounce here, that's why we can see the lag.

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

• Pass the existing shouldShowSmallScreen (which will work for both mobile screen width in modal) into here and remove the isInModal.
• Use the shouldShowSmallScreen here
• Pass shouldShowSmallScreen to inside SignInPageContent
• Replace this condition with the shouldShowSmallScreen props, same in here.

2. We should use the useWindowDimensions in those places rather than the withWindowDimensions HOC, we're gradually moving the codebase to using the hook and the HOC will be deprecated soon.

What alternative solutions did you explore? (Optional)

NA

[ishpaul777]

@tienifr, it seems like passing shouldShowSmallScreen is somewhat similar to passing isInModal, given that isSmallScreenWidth is already being passed to SignInPageContent from the HOC. While transitioning to hooks from the HOC might be considered an improvement, it's not necessarily required to address the issue, but it could be a nice-to-have enhancement. Please don't take this the wrong way, but I'd have to stand by my proposal

[tienifr]

@ishpaul777 let's leave to the C+ to review, thanks!

While transitioning to hooks from the HOC might be considered an improvement, it's not necessarily required to address the issue, but it could be a nice-to-have enhancement

I think we should fix this here since it's very laggy during my testing, it's not anything complicated.

[adelekennedy]

@thesahindia to review the above!

[adelekennedy]

@thesahindia lil' bump

[melvin-bot]

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

[thesahindia]

@ishpaul777's proposal was first. @tienif suggested better code but the solution is same. They also suggested an improvement. I am fine with both. So I will leave the judgment to the engineer.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[tienifr]

There's another issue that I notice while investigating: When resizing the screen from Desktop to Mobile view, there's a significant lag in the sign ui changes, the UI stays as mobile UI for some time, then changes to the Desktop UI

@puneetlath Just a note that my proposal is the only one that also fixes this lagging issue in this flow. I think we should fix it as part of this issue for better UX.

[ishpaul777]

I'm fine with any decision that @puneetlath will make, but I would appreciate some clarification. Are we addressing the issue of missing animation or focusing on a performance improvement problem? There are other components that also use the withWindowDimensions HOC, i think it would make more sense to have a seprate issue to address this more holistically, if @tienifr should post this in #open-source channel with Problem/Solution template that would be more helpful.

Quoting from the guidelines.

ALL NEW PROPOSALS MUST BE DIFFERENT FROM EXISTING PROPOSALS. The difference should be important, meaningful or considerable.

here the difference is a improvement suggestion that isn't related for original issue

[tienifr]

Are we addressing the issue of missing animation or focusing on a performance improvement problem?

I think we should do app lagging fixes/major performance improvement anywhere we can, especially if it's a low-hanging fruit and affects the current flow in the issue. It only makes things better without downsides.

[ishpaul777]

I think the Proposal should be selected on the base of solving the Original issue not extra nice-to-have improvements. Rest is up to internal engineer to review, i will respect the decision. @puneetlath do you mind commenting on situation when you get the chance. Thanks!

[melvin-bot]

@puneetlath, @adelekennedy, @thesahindia Huh... This is 4 days overdue. Who can take care of this?

[puneetlath]

Sorry for the delay. I prefer @tienifr's solution. I think it leaves the code in a better place than it was before, while fixing the issue.

I appreciate your effort here @ishpaul777 and you're a valued contributor. My general approach is to try and choose the best proposal without regard to which came first. Looking forward to your next contribution.

[melvin-bot]

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

[melvin-bot]

📣 @tienifr 🎉 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]

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

[melvin-bot]

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

[melvin-bot]

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

• fix: 30252 Animation is not visible on desktop but visible on Mobile in sign in Modal #30954

If no regressions arise, payment will be issued on 2023-11-30. 🎊

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:

• @thesahindia does not require payment (Eligible for Manual Requests)
• @tienifr requires payment offer (Contributor)
• @ishpaul777 requires payment offer (Reporter)

[melvin-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:

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

[adelekennedy]

Payouts due:

Issue Reporter: $50 @ishpaul777 (reported on the 23rd)
Contributor: $500 @tienifr (Upwork)
Contributor+: $500 @thesahindia (NewDot)

[thesahindia]

I don't think we need a test case for this.

[JmillsExpensify]

$500 payment approved for @thesahindia based on comment above.