[HOLD for payment 2023-01-11] [$2000] [Dark mode] Scrollbars should have a dark color

[aldo-expensify]

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:

1. In the comment composer, click the Emoji button
2. Verify that the scroll bar looks "well" in the dark mode

Expected Result:

Scrollbar should be darker

Actual Result:

Scrollbar color is toooo light

Workaround:

N/A this is just cosmetic

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number:
Reproducible in staging?:
Reproducible in production?:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~019481609abc2ae0b8
• Upwork Job ID: 1603152076507746304
• Last Price Increase: 2022-12-21

[melvin-bot]

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

[aldo-expensify]

cc @shawnborton do we have a color for the scrollbars?

[aldo-expensify]

Do we have a tracking issue for things related to dark mode?

[shawnborton]

Oh interesting, I would think we would hide the scroll bar in that particular piece of UI. Thoughts?

[shawnborton]

Otherwise I would make the scroll background the same color as the darkest app BG color, and the tracking button the same color as our default buttons.

I don't think we have a tracking issue for things related to dark mode... yet? I am not sure if we need one though, we can just handle bugs on a 1:1 basis. Now that I see the scroll bar some more, I can see why it would be useful given how long the emoji list is.

[aldo-expensify]

Oh interesting, I would think we would hide the scroll bar in that particular piece of UI. Thoughts?

Maybe, but you still have the scrollbar in other views

[shawnborton]

Good point. Okay, so let's try with my color suggestions above?

[aldo-expensify]

Good point. Okay, so let's try with my color suggestions above?

sounds good to me!

[grgia]

I could bundle these dark mode color fixes into one PR for faster QA, mind if I take this one too?

[aldo-expensify]

I could bundle these dark mode color fixes into one PR for faster QA, mind if I take this one too?

Feel free to take them if there is no engineer assigned ;)

[melvin-bot]

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

[grgia]

Haven't gotten to this one yet

[melvin-bot]

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

[grgia]

Looking into this, will update with what I find

[grgia]

https://dev.to/shayanypn/anything-about-customizing-native-scrollbar-59hm dropping this here to reference later

[grgia]

@marcaaron I saw you updated some WebKit styles, do you have insight on modifying the scrollbar colors

[marcaaron]

Scrollbars... I would check this for information about web ->

https://css-tricks.com/the-current-state-of-styling-scrollbars-in-css/

I am not really sure about react native...

I found this -> https://reactnative.dev/docs/scrollview#indicatorstyle-ios

But it's iOS only.

There's also a package someone created to customize the indicators here. Which could be a sign that this is not an easy thing to do. But I'd be surprised if we are the first people to ever try to implement a "dark mode" in a React Native app 😅

[grgia]

Thanks for the direction, Marc!
For later reference, this is the GitHub dark mode scrollbar hover:

[sakluger]

Thanks Shawn! Sounds like we're good to accept @0xmiroslav's proposal.

[JmillsExpensify]

@aimane-chnaif @grgia would you like to see an updated version of @0xmiroslav proposal before we move to the PR phase? If not, let's get them assigned building on @sakluger's last comment.

[aimane-chnaif]

@aimane-chnaif @grgia would you like to see an updated version of @0xmiroslav proposal before we move to the PR phase? If not, let's get them assigned building on @sakluger's last comment.

@JmillsExpensify you mean this one?

[JmillsExpensify]

Yes

[aimane-chnaif]

yes checked. So this may also fix @quinthar's concern in android (slack convo)
cc: @cristipaval (for #13815) @Julesssss (for #13087)

[grgia]

I think that we are good to move forward. I'll assign @0xmiroslav

[melvin-bot]

📣 @0xmiroslav You have been assigned to this job by @grgia!
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 📖

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[syedsaroshfarrukhdot]

This PR solving this issue is causing a Deploy Blocker #13951 Currently as the issue is locked so commenting here and tagging the issue. Already proposed solution on Slack.

To fix it we need to set color to colorBackgroundFloating to fix the issue in styles.xml because applying background color dark to android app also causes Modals to be Dark.

As it is a Deploy Blocker I know the urgency to fix the issue so I will be available to raise a PR in next few minutes if assigned to the job.

diff --git a/android/app/src/main/res/values/styles.xml b/android/app/src/main/res/values/styles.xml
index a804c14ab..b30501c5b 100644
--- a/android/app/src/main/res/values/styles.xml
+++ b/android/app/src/main/res/values/styles.xml
@@ -10,6 +10,7 @@
         <item name="android:statusBarColor">#061B09</item>
         <item name="colorAccent">@color/accent</item>
         <item name="android:editTextBackground">@drawable/rn_edit_text_material</item>
+        <item name="colorBackgroundFloating">@color/white</item>
     </style>
 
     <!-- Themes unique to the boot splash page -->

After Fix :-

WhatsApp.Video.2023-01-03.at.11.13.45.AM.1.mp4

[0xmiros]

I explained details here regarding #13951

[0xmiros]

Interestingly, I'm not seeing this issue on Android 13. I'm on app version 1.2.48-2 and I see the dark window background no matter how the app is opened (from background, fresh open after kill, fresh open after back button press):

@Julesssss regarding your concern 👆, I explained here 👇:

This also fixes any other dark mode issues in android like #13815, #13087 and also when opening the keyboard, the background flashes white reported in slack

[0xmiros]

and that PR is already deployed to production

[sakluger]

The issue was deployed to production, but it seems like some of the automation isn't triggering, so I updated the title manually.

For payment:

• Issue reporter: There is no external issue reporter
• Contributor: @0xmiroslav I've sent an offer through Upwork
• C+ reviewer: @aimane-chnaif I've sent an offer through Upwork

It looks like the linked PR was merged within 3 days, so I believe that this is eligible for a 50% bonus.

It also looks like there may have been a regression from the PR, but I can't tell if that was actually a regression or a false positive. @grgia @aimane-chnaif could you please confirm if there was a regression or not that would affect payment?

[aimane-chnaif]

It is a kind of regression but PR not reverted. And it's handled in separate issue here with expanded scope and removed from deploy blockers list.
So I think we can consider it's not a regression.
Thoughts? @grgia

[sakluger]

Hey everyone, I discussed with my team regarding the payments, and I have an update. For this issue, we still consider the regression to be an actual regression - the PR doesn't necessarily need to be reverted to be deemed a regression. For that reason, we will be applying the 50% penalty for the regression.

Additionally, we do not pay the "efficiency" bonus for a fast merge when there is a regression.

Given the above info, here are the payments that I will be sending out today:

• Contributor: @0xmiroslav - $2000 payment
• C+ reviewer: @aimane-chnaif - $1000 payment

[sakluger]

The contributors have been paid, the contracts completed, and the Upwork post closed.

[mallenexpensify]

Late to this but... for the timeliness bonus, @0xmiroslav was hired on hired on 12/28 and the PR was merged on 12/30. CONTRIBUTING.md just got updated today to state

If the PR causes a regression within 7 days of being deployed to production, contributors are not eligible for the 50% bonus.

Before then we didn't make clearly state that a regression (reverted or not) would affect the timeliness bonus. With CONTRIBUTING.md being the source of truth, we need to always be able to point to it (which we can now). I've issued @0xmiroslav and @aimane-chnaif each $1000 bonus for timeliness.

For "If a regression is found but not reverted, does it still affect compensation?" the answer is yes for C+, per our internal process doc for C+, the regression is the trigger, regardless if it's reverted or not.

the PR has been deployed to production with no regressions. If regressions are found that “should have”* been caught after the PR has been approved

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.