Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$1000] [HOLD for payment 2023-06-05] Android status bar colour doesn't match the background #13001

Closed
Julesssss opened this issue Nov 24, 2022 · 46 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff

Comments

@Julesssss
Copy link
Contributor

Julesssss commented Nov 24, 2022

Problem

While resolving this related issue I uncovered a new problem. The LHN page background colour on mobile does not match the other pages in the app. The background colour is slightly different, and this is because of the need to display LHN and chat components side by side on desktop/web.

While on the LHN page, the status bar is visible. Additionally, the LHN background is different from every other page in the app. This will be far more noticeable once we switch to the dark theme:

Dark Theme
dark1
dark2
dark3
dark4

Light Theme
Screenshot_20221124_171947

Screenshot_20221124_145436

Solution

As discussed here in Slack, we have a couple of options:

  1. Do nothing, live with the unique background colour for mobile. I don't like this as we are breaking mobile because of an issue that only exists on web/desktop (the LHN is not distinguishable when side by side)

  2. Ignore the background colour difference, but implement a hack on Android that forces the status bar to not render, therefore resolving part of the problem. I think this option is bad as it introduces technical debt that is difficult to understand, even for someone very familiar with native Android code. Making changes to the status bar in the future will be difficult as the hack is pretty much un-discoverable without finding the introducing PR

  3. At the React Native layer, display a different background colour on the LHN depending on the platform. This trades an inconsistency within the mobile apps, to an inconsistency based on whether we display the sidebar. We already use platform-specific code and set attributes based on the screen size, so this feels like the ideal solution to me.

background: props.isSmallScreenSize styles.sidebar : styles.sidebarLarge
  1. Programatically set the status bar colour natively. While this would certainly work, we'd have to do it on every page and it is not immediate. We would see the default white status bar appear briefly before the green colour is set.

  2. Refactor the app so that StatusBar doesn't sit above the entire app. This way we could set the colour based on context from the current page. This is a large refactor though, and I don't think this is a good use of time.

  3. Spend time looking for a solution that lets us style each page on every platform. This is likely to be a native Android solution, but there's a good chance we won't find a better solution than those listed above. There is no guarantee this is possible, and it could take weeks of work.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01721757a40a8d7d9c
  • Upwork Job ID: 1666788536927301632
  • Last Price Increase: 2023-06-08
@Julesssss Julesssss self-assigned this Nov 24, 2022
@Julesssss Julesssss added Daily KSv2 Internal Requires API changes or must be handled by Expensify staff Improvement Item broken or needs improvement. labels Nov 24, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 28, 2022

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

@melvin-bot melvin-bot bot added the Overdue label Nov 28, 2022
@Julesssss Julesssss added Weekly KSv2 and removed Daily KSv2 labels Nov 28, 2022
@melvin-bot melvin-bot bot removed the Overdue label Nov 28, 2022
@Julesssss
Copy link
Contributor Author

Awaiting the merging of dark mode before I investigate further.

@Julesssss
Copy link
Contributor Author

Returning to this soon after the notifications doc.

@melvin-bot melvin-bot bot removed the Overdue label Dec 12, 2022
@Julesssss Julesssss added Monthly KSv2 and removed Weekly KSv2 labels Dec 12, 2022
@Julesssss Julesssss added Weekly KSv2 and removed Monthly KSv2 labels Jan 5, 2023
@melvin-bot melvin-bot bot added the Overdue label Jan 13, 2023
@Julesssss
Copy link
Contributor Author

Focusing on this once I'm back from OOO

@melvin-bot melvin-bot bot removed the Overdue label Jan 16, 2023
@Julesssss Julesssss added Monthly KSv2 Weekly KSv2 and removed Weekly KSv2 Monthly KSv2 labels Jan 16, 2023
@melvin-bot melvin-bot bot added the Overdue label Jan 30, 2023
@Julesssss
Copy link
Contributor Author

No change here.

@melvin-bot melvin-bot bot changed the title Android status bar colour doesn't match the background [HOLD for payment 2023-06-05] Android status bar colour doesn't match the background May 29, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label May 29, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 29, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented May 29, 2023

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

If no regressions arise, payment will be issued on 2023-06-05. 🎊

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

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 melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Jun 5, 2023
@Julesssss Julesssss added the External Added to denote the issue can be worked on by a contributor label Jun 8, 2023
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2023-06-05] Android status bar colour doesn't match the background [$1000] [HOLD for payment 2023-06-05] Android status bar colour doesn't match the background Jun 8, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 8, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01721757a40a8d7d9c

@melvin-bot
Copy link

melvin-bot bot commented Jun 8, 2023

Triggered auto assignment to @lschurr (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Jun 8, 2023

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

@Julesssss Julesssss removed the External Added to denote the issue can be worked on by a contributor label Jun 8, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 8, 2023

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

@Julesssss
Copy link
Contributor Author

Hey Lauren, the PR was already merged by @janicduplessis, who's with Margelo I believe. So we just need to pay out

@lschurr
Copy link
Contributor

lschurr commented Jun 8, 2023

Sorry who needs to be paid for this one @Julesssss? It looks like maybe @mananjadhav and @aimane-chnaif worked on this PR? #15778

@Julesssss
Copy link
Contributor Author

Oh yeah, my bad:

@mrousavy
Copy link
Member

mrousavy commented Jun 9, 2023

For Margelo we can discuss over our Slack channel :)

@lschurr
Copy link
Contributor

lschurr commented Jun 9, 2023

Cool @aimane-chnaif and @mananjadhav - please apply to the job: https://www.upwork.com/jobs/~01721757a40a8d7d9c

@mananjadhav
Copy link
Collaborator

@lschurr Same session issue here too. Would you please be able to send an invite here too? Thanks for helping out.

@melvin-bot melvin-bot bot added the Overdue label Jun 12, 2023
@lschurr
Copy link
Contributor

lschurr commented Jun 12, 2023

@mananjadhav - I invited you to the job.

@melvin-bot melvin-bot bot removed the Overdue label Jun 12, 2023
@mananjadhav
Copy link
Collaborator

Applied @lschurr

@lschurr
Copy link
Contributor

lschurr commented Jun 13, 2023

This is paid. Closing.

@lschurr lschurr closed this as completed Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff
Projects
None yet
Development

No branches or pull requests

8 participants