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.33k stars 2.76k forks source link

[HOLD for payment 2024-01-25] [$500] Bank account - Missing white background when Plaid modal is rendering #32685

Closed kbecciv closed 7 months ago

kbecciv commented 9 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.9.2 Reproducible in staging?: y Reproducible in production?: n 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.
  3. Go to Bank account.
  4. Click Connect online with Plaid.

Expected Result:

When Plaid modal is rendering, a white background will appear with a spinner (PROD behavior).

Actual Result:

When Plaid modal is rendering, the background is transparent instead of white., which now looks like having two spinners on the same page.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

Staging

https://github.com/Expensify/App/assets/93399543/731a3098-02f2-4fc5-98c7-e5314c386363

Production

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0190435a1b9eb3175f
  • Upwork Job ID: 1732815115354394624
  • Last Price Increase: 2023-12-07
  • Automatic offers:
    • alitoshmatov | Reviewer | 28064602
    • tienifr | Contributor | 28064603
Issue OwnerCurrent Issue Owner: @muttmuure
github-actions[bot] commented 9 months 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 9 months ago

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

neil-marcellini commented 9 months ago

Taking a look πŸ‘€

neil-marcellini commented 9 months ago

Nothing really jumps out to me from the list of PRs in this release, but maybe the theme switching has to do with it. I'll do a little investigation locally now.

neil-marcellini commented 9 months ago

Shoot, I'm really not sure what's going on here. The view is rendered by react-plaid-link in an iframe. I was able to pause and inspect the page by setting a debugger below this line, but I haven't figure out the root cause. My guess is that we somehow broke this when changing some of the root styles of the app. I'll ask for help in the Slack open source channel.

image

neil-marcellini commented 9 months ago

Posted in Slack here

melvin-bot[bot] commented 9 months ago

Job added to Upwork: https://www.upwork.com/jobs/~0190435a1b9eb3175f

melvin-bot[bot] commented 9 months ago

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

ZhenjaHorbach commented 9 months ago

The problem is related to this PR https://github.com/Expensify/App/pull/32259 I added these lines and the background returned)

https://github.com/Expensify/App/pull/32259/files#diff-8f62b6ced28d3396b501d2e89a2e7cb761d16cd7dc977aebece03d4a5da5c24eL34-L36

neil-marcellini commented 9 months ago

This issue isn't really a big deal so it shouldn't block the deploy

grgia commented 9 months ago

We cannot use root styling anymore, so we will need to handle this another way

grgia commented 9 months ago

cc @chrispader

tienifr commented 9 months ago

Proposal

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

When Plaid modal is rendering, the background is transparent instead of white., which now looks like having two spinners on the same page.

What is the root cause of that problem?

We don't have color-scheme: dark !important; on :root any more so this happens.

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

Add color-scheme: dark !important; in CSS to target the iframe of Plaid. We've done similar things with Onfido as well.

What alternative solutions did you explore? (Optional)

We can make the color scheme dynamic based on theme.colorScheme as well, but still targeting the iframe of Plaid.

grgia commented 9 months ago

@tienifr we cannot add that line back because we are adding a light theme to App

tienifr commented 9 months ago

@grgia we add it back but only targeting the Plaid iframe, it won't affect other components, or do you mean there'll be light theme for the Plaid background as well?

grgia commented 9 months ago

I more mean I think we should avoid any hardcoded colors via CSS and use a ColorSchemeWrapper instead. But you're correct that that would work for this specific case if we only target the iframe

tienifr commented 9 months ago

@grgia yeah but Plaid is 3rd party code so it won't work with our custom components, unless we ask the library authors to add a prop like colorScheme to their exposed hook, which I'm not sure they'll agree to.

I think it's better we treat it as 3rd party code and have a CSS file like we did for Onfido here, and make sure it doesn't impact other components, which will of course follow our color scheme pattern.

cc @chrispader

grgia commented 9 months ago

@tienifr yeah given that it's a loading screen and should be white on all platforms, I think that's fine

tienifr commented 9 months ago

@tienifr yeah given that it's a loading screen and should be white on all platforms, I think that's fine

@alitoshmatov FYI we have an alignment here and my proposal will work well for that

alitoshmatov commented 9 months ago

That make sense.

We can go with @tienifr 's proposal

C+ reviewed πŸŽ€ πŸ‘€ πŸŽ€

melvin-bot[bot] commented 9 months ago

Current assignee @neil-marcellini is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

melvin-bot[bot] commented 9 months ago

@neil-marcellini, @alitoshmatov Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

neil-marcellini commented 9 months ago

I'll try to review this soon

neil-marcellini commented 9 months ago

Ran out of time the last few days, on my list for today.

neil-marcellini commented 9 months ago

We can go with @tienifr 's proposal

C+ reviewed πŸŽ€ πŸ‘€ πŸŽ€

~@tienifr would you please update your proposal explaining that we'll use a specific CSS file like we did for Onfido? I'll hire you now to prevent any delays.~

Oh never mind, it's fairly clear already.

melvin-bot[bot] commented 9 months ago

πŸ“£ @alitoshmatov πŸŽ‰ An offer has been automatically sent to your Upwork account for the Reviewer role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 9 months ago

πŸ“£ @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 πŸ“–

tienifr commented 8 months ago

PR ready for review https://github.com/Expensify/App/pull/33337.

melvin-bot[bot] commented 8 months ago

This issue has not been updated in over 15 days. @neil-marcellini, @alitoshmatov, @tienifr eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

melvin-bot[bot] commented 8 months ago

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

melvin-bot[bot] commented 8 months ago

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

If no regressions arise, payment will be issued on 2024-01-25. :confetti_ball:

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

alitoshmatov commented 7 months ago

This is ready for payment

alitoshmatov commented 7 months ago

I think there is no bugzero member assigned to handle payment here

cc: @grgia @neil-marcellini

neil-marcellini commented 7 months ago

Yep thanks, assigning one

melvin-bot[bot] commented 7 months ago

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

melvin-bot[bot] commented 7 months ago

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

muttmuure commented 7 months ago

Handling

muttmuure commented 7 months ago

Everyone is paid