-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[RNMobile] Try dark mode (iOS) #17067
Changes from 3 commits
7ebe160
8a4a8a4
8958ff6
a1a26a5
8bb6d0e
aa61ee7
d3c7eef
0d8f630
580b63b
43bc042
d79f027
c096c3c
c826cf1
16ad3d8
8654cb1
2809ebf
67cefed
8cb3708
bf2a1b5
3f71f3d
5aee1ce
852fa05
327d545
de173df
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { eventEmitter, initialMode } from 'react-native-dark-mode'; | ||
import React from 'react'; | ||
|
||
export function useStyle( light, dark, theme ) { | ||
const finalDark = { | ||
...light, | ||
...dark, | ||
}; | ||
|
||
return theme === 'dark' ? finalDark : light; | ||
} | ||
|
||
// This function takes a component... | ||
export function withTheme( WrappedComponent ) { | ||
return class extends React.Component { | ||
constructor( props ) { | ||
super( props ); | ||
|
||
this.state = { | ||
mode: initialMode, | ||
}; | ||
} | ||
|
||
componentDidMount() { | ||
eventEmitter.on( 'currentModeChanged', ( newMode ) => { | ||
this.setState( { mode: newMode } ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personal preference, but if |
||
} ); | ||
} | ||
|
||
render() { | ||
return <WrappedComponent theme={ this.state.mode } { ...this.props } />; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think your approach looks really good and clean @etoledom . 🎉 I don't see any issues. I had a couple of thoughts as I was looking at it that I'll include here just as food for thought. Since the One other thing that I was wondering is if long-term the dark/light "theme" is something that we should put in the redux store. I'm honestly not sure, and I think your current approach is better at this time because it is both simpler and easier to change if/when we need to. Again, these are just a couple of thoughts I had as I was looking it over that I wanted to share. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for sharing your thoughts!
I like that idea. It's been quite annoying the need to pass the third argument all the time. But there has been a couple of cases where having the current theme has been helpful, cases where the color is given by html class name. But we still can pass both the function and the theme itself I guess :) I'll do some experiments about this. Thanks again! |
||
} | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,17 @@ | ||
|
||
.rich-text { | ||
.richText { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just asking so I know, is camelcase the preferred convention for our There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if it's a convention already, but I have seen more camel-cases on mobile scss lately. And it's easier to work with on JS files. The snake-cased ones seem to be older ones. So I took the liberty to modify some that were needed on this PR. |
||
font-family: $default-regular-font; | ||
min-height: $min-height-paragraph; | ||
color: $gray-900; | ||
text-decoration-color: $blue-500; | ||
} | ||
|
||
.richTextDark { | ||
color: $white; | ||
text-decoration-color: $blue-500; | ||
background-color: #1c1c1e; | ||
} | ||
|
||
.rich-text-placeholder { | ||
color: $gray; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR, but another possibly interesting idea would be to somehow dynamically get the dark style when appropriate from the style object. I haven't given this enough thought to say it's a good or even decent idea (especially since your current implementation already seems pretty good), but it's interesting, and I just wanted to throw it out there.
Psuedocode:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a very interesting idea. I gave a couple of seconds to similar possibilities, and definitely something to look for on a next iteration!
Thanks for the suggestion! 🙏