[HOLD for payment 2022-12-15] [$1000] Light background colour is shown when navigating between right docked modal pages

[Julesssss]

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:

• Open a right docked page on web or desktop
• Navigate deeper into another right docked modal
• Navigate back and forth between them

Expected Result:

• We shouldn't see an obviously incorrect colour as the background

Actual Result:

• We see an obviously incorrect colour as the background (this existed before, but is much more jarring with the darker colours.)

Platform:

Where is this issue occurring?

• Web
• Desktop App

Reproducible in staging?: yes
Reproducible in production?: yes, pretty soon
Email or phone of affected tester (no customers): all

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f7b6e9d9a74ed27a
• Upwork Job ID: 1598132781569765376

[melvin-bot]

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

[tienifr]

Proposal

Problem

In

App/src/libs/Navigation/NavigationRoot.js

Line 19 in 1397052

background: themeColors.highlightBG,

we're setting background of navigation is #002E22

Solution

I'm not sure the correct color is, but we can change it In https://github.com/Expensify/App/blob/13970529db79931c2371f7a0b6d5f96792e08c60/src/libs/Navigation/NavigationRoot.js like:

const navigationTheme = {
    ...DefaultTheme,
    colors: {
        ...DefaultTheme.colors,
-        background: themeColors.highlightBG,
+        background: 'transparent',
    },
};

Screen.Recording.2022-11-29.at.17.49.11.mov

[melvin-bot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[isabelastisser]

Not overdue.

[shonsirsha]

Proposal

A lighter bg colour is shown when navigating between right-docked modal pages, e.g: Workspaces.

To fix this, we could introduce a minimal change - which is the following:

In src/libs/Navigation/NavigationRoot.js:

const navigationTheme = {
    ...DefaultTheme,
    colors: {
        ...DefaultTheme.colors,
-        background: themeColors.highlightBG,
+        background: themeColors.appBG,
    },
};

This maintains the background colour of NavigationContainer to be the same as the ModalStackNavigators, therefore a different (incorrect) colour won't be shown.

Web:

Screen.Recording.2022-12-01.at.04.56.06.mov

Desktop:

Screen.Recording.2022-12-01.at.05.03.12.mov

A lighter bg colour is not shown anymore ✅

Thanks! 🙂

[rushatgabhane]

This is a matter of personal preference. I prefer appBG over transparent

@shawnborton which one do you prefer?

[rushatgabhane]

I'm not sure the correct color is, but we can change it In

No matter the background color preference, I think we should hire @tienifr for this job because they were first and changing the color is trivial.

[shawnborton]

Nice, yeah I agree we should use the appBG color here

[rushatgabhane]

@Julesssss let's hire @tienifr if you agree with #13086 (comment)

🎀 👀 🎀 C+ reviewed

[rushatgabhane]

thanks shawn

[Julesssss]

Ideal 👍

[melvin-bot]

📣 @tienifr You have been assigned to this job by @Julesssss!
Please apply to this job in Upwork 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]

Hi @rushatgabhane I just created The PR. Please help to review it

[melvin-bot]

@Julesssss, @rushatgabhane, @isabelastisser, @tienifr Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[Julesssss]

Merged awaiting deployment

[isabelastisser]

Update:

@tienifr and @rushatgabhane, I hired both of you for the job in Upwork.

[melvin-bot]

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

• Fix/13086: Light background colour is shown when navigating between right docked modal pages #13242

If no regressions arise, payment will be issued on 2022-12-15. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

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

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]

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:

• [@rushatgabhane / @Julesssss] The PR that introduced the bug has been identified. Link to the PR:
• [@rushatgabhane / @Julesssss] 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:
• [@rushatgabhane / @Julesssss] 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:
• [@isabelastisser] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here: regression test case not recommended, context here.

[rushatgabhane]

This was always present, right? The color difference just became more apparent when we switched to dark mode

[Julesssss]

This was always present, right? The color difference just became more apparent when we switched to dark mode

Yeah, correct.

[isabelastisser]

Update: @tienifr and @rushatgabhane were paid!

The contract ended in Upwork.

[isabelastisser]

@Julesssss @rushatgabhane can you please update the checklist above? Thanks!

[Julesssss]

Hi @isabelastisser, I didn't check the boxes initially because it's not relevant here. This wasn't a regression and I think the checklist is often incorrectly added to every issue.

[isabelastisser]

Thanks for the heads up, @Julesssss ! Closing the issue then.

[tienifr]

@isabelastisser Hi, this issue has Merged PR within 3 business days of assignment, am I eligible for the 50% bonus?

[isabelastisser]

@tienifr @rushatgabhane yes, I issued the bonus now. Thank you!