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.35k stars 2.78k forks source link

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

Closed aldo-expensify closed 1 year ago

aldo-expensify commented 1 year 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!


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

image

Workaround:

N/A this is just cosmetic

Platform:

Where is this issue occurring?

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[bot] commented 1 year ago

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

aldo-expensify commented 1 year ago

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

aldo-expensify commented 1 year ago

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

shawnborton commented 1 year ago

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

shawnborton commented 1 year ago

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 commented 1 year ago

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

image

shawnborton commented 1 year ago

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

aldo-expensify commented 1 year ago

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

sounds good to me!

grgia commented 1 year ago

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

aldo-expensify commented 1 year ago

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[bot] commented 1 year ago

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

grgia commented 1 year ago

Haven't gotten to this one yet

melvin-bot[bot] commented 1 year ago

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

grgia commented 1 year ago

Looking into this, will update with what I find

grgia commented 1 year ago

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

grgia commented 1 year ago

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

marcaaron commented 1 year ago

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 commented 1 year ago

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

Screenshot 2022-12-06 at 9 06 51 AM
melvin-bot[bot] commented 1 year ago

@grgia Whoops! This issue is 2 days overdue. Let's get this updated quick!

grgia commented 1 year ago

Working on a few other issues, if I can't get to this in the next day or so, I'll make external

grgia commented 1 year ago

Gonna make this external

melvin-bot[bot] commented 1 year ago

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

grgia commented 1 year ago

Not sure what happened with the overdue label

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~019481609abc2ae0b8

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

getusha commented 1 year ago

it is not possible to change the color of a scrollbar in a FlatList in React Native on Android. for the IOS i see dark scroll bar,

https://user-images.githubusercontent.com/75031127/207785742-d66ce5c8-e452-4b61-9486-e2e73c368dfd.mov

so we can implement this only for the web / desktop?

What's your thoughts on this @marcaaron

bernhardoj commented 1 year ago

Proposal

We can add scrollbar styling to the html in web/index.html inside <style> tag.

html,
body,
#root,
#root > div,
#root > div > div {
+   scrollbar-color: #061B09 #184E3D;
    height: 100% !important;
}
+::-webkit-scrollbar {
+    background-color: #061B09;
+}
+::-webkit-scrollbar-thumb {
+    background-color: #184E3D;
+}

scrollbar-color only works on Firefox, so we need to also add -webkit-scrollbar-* for Chromium and WebKit browser.

On Android, the scrollbar also already had a dark color.

Result

https://user-images.githubusercontent.com/50919443/207793452-8e6d41e0-0803-4e27-bd00-5148098be0bb.mov

https://user-images.githubusercontent.com/50919443/207794508-37c06dd2-8e76-43d9-b1e9-3d3ffee9f154.mp4

getusha commented 1 year ago

@bernhardoj how do you handle the switch to the light theme?

dhanashree-sawant commented 1 year ago

@getusha Working on the CSS fix, can you let me know how do I change it to light theme on web?

dhanashree-sawant commented 1 year ago

Proposal

Include the following CSS inside style of web/index.html. It works for chrome and safari:

*::-webkit-scrollbar {      
       
        width: 10px;
}

*::-webkit-scrollbar-thumb {
        background-color: rgba(255,255,255,0.5);
        border-radius: 10px;
    border: 1px solid rgba(255,255,255,0.6);
}

*::-webkit-scrollbar-track {
    background-color: rgba(255,255,255,0.05);
    border: 1px solid grey;
    border-radius: 10px;
}

Screenshot 2022-12-15 at 12 38 38 PM

Will modify the proposal once I get to know how to switch between light and dark mode

Note: Width element for scrollbar is required else changes won't be visible

getusha commented 1 year ago

Proposal


diff --git a/src/styles/styles.js b/src/styles/styles.js
index baac1d8f33..8c5b7b0aef 100644
--- a/src/styles/styles.js
+++ b/src/styles/styles.js
@@ -19,6 +19,7 @@ import pointerEventsNone from './pointerEventsNone';
 import pointerEventsAuto from './pointerEventsAuto';
 import overflowXHidden from './overflowXHidden';
 import CONST from '../CONST'; 

+    scrollbarDark: {
+        scrollIndicatorInsets: {
+          top: -10,
+          right: -10,
+          bottom: -10,
+          left: -10,
+        },
+        backgroundColor: '#051c09',
+      }

diff --git a/src/components/EmojiPicker/EmojiPickerMenu/index.js b/src/components/EmojiPicker/EmojiPickerMenu/index.js
index 2d97a24439..268746dc50 100755
--- a/src/components/EmojiPicker/EmojiPickerMenu/index.js
+++ b/src/components/EmojiPicker/EmojiPickerMenu/index.js
@@ -1,5 +1,5 @@

@@ -502,6 +502,7 @@ class EmojiPickerMenu extends Component {
                             style={[
                                 styles.emojiPickerList,
                                 this.isMobileLandscape() && styles.emojiPickerListLandscape,
+                                styles.scrollbarDark
                             ]}
                             extraData={
                               [this.state.filteredEmojis, this.state.highlightedIndex, this.props.preferredSkinTone]

Result :tada:

https://user-images.githubusercontent.com/75031127/207797277-d2d5f6ea-5a71-4d07-a5d4-7fcc183f8365.mov

or complete black

https://user-images.githubusercontent.com/75031127/207798571-a497e3dc-704d-4149-9657-c04db906675a.mov

Oh Keep our theme color 😅

https://user-images.githubusercontent.com/75031127/207800388-e32271a0-aa58-456d-97f5-99c5bc2bd579.mov

On Desktop

https://user-images.githubusercontent.com/75031127/208238432-4ca18431-d0e9-46e5-bf30-ee16062bcb5d.mov

update: Working on

priyeshshah11 commented 1 year ago

Proposal

OG proposal here https://github.com/Expensify/App/issues/13592#issuecomment-1354451969

On iOS it is already dark, so this only needs to be fixed on web. The solution below works for all browsers including Safari, Chrome & Firefox. I have put in light green colour below as per #13592 but we can substitute it with any colour the design team wants.

In src/web/index.html

diff --git a/web/index.html b/web/index.html
index d7074a03d..1f1083030 100644
--- a/web/index.html
+++ b/web/index.html
@@ -63,6 +63,22 @@
             -webkit-text-fill-color: #ffffff;
             caret-color: #ffffff;
         }
+        /* Customize scrollbar color & width for firefox */
+        body {
+            scrollbar-width: thin;
+            scrollbar-color: #03d47c transparent;
+        }
+        /* Customize scrollbar color & width for most browsers except firefox */
+        ::-webkit-scrollbar {
+            width: 4px;
+        }
+        ::-webkit-scrollbar-track {
+            background-color: transparent;
+        }
+        ::-webkit-scrollbar-thumb {
+            background-color: #03d47c;
+            border-radius: 4px;
+        }
     </style>
     <meta name="viewport" content="width=device-width, user-scalable=no, initial-scale=1">
     <link rel="shortcut icon" id="favicon" href="/favicon.png">

@grgia @aimane-chnaif

Chrome

Screen Shot 2022-12-17 at 9 02 55 PM

Firefox

https://user-images.githubusercontent.com/38547776/208236614-907a4b07-e2ac-41ed-8666-ddcb302cfba3.mov

Safari

https://user-images.githubusercontent.com/38547776/208236852-3aa69544-4a56-4ed5-94e0-5105f0358b71.mov

getusha commented 1 year ago

In the future we need to switch between themes and we don't want to have same scroll bar colors for themes right? how are we going to change the style dynamically if we're applying this on index.html?

priyeshshah11 commented 1 year ago

In the future we need to switch between themes and we don't want to have same scroll bar colors for themes right? how are we going to change the style dynamically if we're applying this on index.html?

@getusha I didn't know that was a requirement, could you please point me to where that requirement is listed?

getusha commented 1 year ago

@priyeshshah11 I am just asking. IMO even if it is not listed it's obvious to think about that, don't you think?

priyeshshah11 commented 1 year ago

@priyeshshah11 I am just asking. IMO even if it is not listed it's obvious to think about that, don't you think?

No I don't think it's obvious, and I don't think that's in the scope of this issue as it applies to a lot of things in the app which are currently not catered for changing themes, but it's good that you're thinking ahead. To answer your initial question, the styles can be overwritten from anywhere within the app but the exact solution would depend on how theme switching is implemented.

getusha commented 1 year ago

@priyeshshah11 Does that work on the desktop app?

priyeshshah11 commented 1 year ago

@priyeshshah11 Does that work on the desktop app?

yes, works on desktop too.

Screen Shot 2022-12-17 at 10 22 05 PM
s77rt commented 1 year ago

Should we hold this until theme-switching is implemented?

aimane-chnaif commented 1 year ago

I don't think we need to handle theme-switching for now, given priority to bug fixes. It's a feature request and should be in TODO list of course. We can do static changes similar to https://github.com/Expensify/App/pull/13164 How do you think? @grgia @shawnborton

shawnborton commented 1 year ago

I agree that we don't need to worry about theme switching for the moment.

aimane-chnaif commented 1 year ago

Also, I think we need to note somewhere all kinds of these issue so we don't miss anything when implement theme switching in the future.

sakluger commented 1 year ago

@grgia @aimane-chnaif it looks like we've gotten a few proposals, do you think we should accept any of these proposals?

aimane-chnaif commented 1 year ago

@grgia @aimane-chnaif it looks like we've gotten a few proposals, do you think we should accept any of these proposals?

I will update tomorrow

aimane-chnaif commented 1 year ago

For native apps, there should be native way to change scroll indicator color that fits dark theme. No one proposed working solution yet.

For iOS, I see some proposals which sets indicatorStyle prop of ScrollView (FlatList) manually but this is limited to one page. There should be more generic solution rather than setting this prop in all usages of scroll views throughout the app.

indicatorStyle

ios

Some proposals indicate that this works already on iOS but NO. If you set device theme to light, this scroll indicator is black and it's difficult to see in dark background.

aimane-chnaif commented 1 year ago

For web, there are several options: (all screenshots below are captured on Windows 10, might look different on other platforms)

Current version: test0

Option 1: (https://github.com/Expensify/App/issues/12914#issuecomment-1352635477) test1

Option 2: (https://github.com/Expensify/App/issues/12914#issuecomment-1352646600) test2

Option 3: (https://github.com/Expensify/App/issues/12914#issuecomment-1356150080) test3

Option 4: This is Github scroll on dark theme what @grgia suggested. test4

Option 1-3 completely customizes scroll bar and it looks quite different than original one (default) on each platform. I like Option 4 because it keeps default scroll view style (i.e. arrow up/down on Windows), though no one proposed this solution yet.

I'd like to hear @shawnborton's suggestions.

bernhardoj commented 1 year ago

Updated Proposal [1]

So, I find another way to change the scrollbar color (for web) similar to GitHub. We can apply color-scheme: dark to the root.

image

image

For android, we can set the scrollbar color to the styles.xml

<style name="BaseAppTheme" parent="Theme.AppCompat.Light.NoActionBar">
    <item name="android:textColor">@color/dark</item>
    <item name="android:colorEdgeEffect">@color/gray4</item>
    <item name="android:statusBarColor">#061B09</item>
    <item name="colorAccent">@color/accent</item>
    <item name="android:editTextBackground">@drawable/rn_edit_text_material</item>
+   <item name="android:scrollbarThumbVertical">@color/scrollBarColor</item>
+   <item name="android:scrollbarThumbHorizontal">@color/scrollBarColor</item>
</style>

The scrollBarColor is a new color with value of #838e85

https://user-images.githubusercontent.com/50919443/209054685-a49343f1-fe37-4945-ada5-edda373198e2.mp4

I'm still finding a way for iOS to set the scrollbar color globally (currently I can only think to create a BaseFlatList which set the indicatorStyle to white and replace all current FlatList with the new BaseFlatList).

priyeshshah11 commented 1 year ago

Option 3: (#12914 (comment)) test3

I'd like to hear @shawnborton's suggestions.

@aimane-chnaif Just wanted to highlight here that my original proposal was for #13592 which was to have a light green coloured scrollbar but I guess the exact colour, width & border-radius can be changed once the design team confirms what they actually want but the idea would still be the same.

@grgia @shawnborton