-
Notifications
You must be signed in to change notification settings - Fork 181
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
Add dark theme setting for settings screen #339
Add dark theme setting for settings screen #339
Conversation
6c3a3b6
to
d456b6c
Compare
import kotlinx.coroutines.flow.StateFlow | ||
|
||
interface SettingViewModel : | ||
UnidirectionalViewModel<SettingViewModel.Event, SettingViewModel.Effect, SettingViewModel.State> { |
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.
Looks like we have a ktlint failure here due to chars are more than 100 in a line.
Exceeded max line length (100)
You can simply break them into multiple lines to pass the GitHub checks.
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.
@anandwana001
Thanks, I fixed it.
58765b4
to
117ccb3
Compare
7718501
to
9b74b7f
Compare
9b74b7f
to
e54d515
Compare
👀 |
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.
Almost LGTM! Cool 🆒
import kotlinx.coroutines.flow.Flow | ||
import kotlinx.coroutines.flow.StateFlow | ||
|
||
interface DroidKaigiAppViewModel : |
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.
Currently, I don't use the name of DroidKaigi other than the theme, so I want to use AppViewModel.
typography = typography, | ||
shapes = shapes, | ||
content = content | ||
) | ||
} | ||
} | ||
|
||
@Composable | ||
private fun colorPalette(theme: Theme?): Colors { |
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.
🆒
@@ -3,4 +3,7 @@ | |||
<string name="error_network">ネットワークエラーが発生しました。もう一度お試しください。</string> | |||
<string name="error_server">サーバーエラーが発生しました。もう一度お試しください。</string> | |||
<string name="error_unknown">予期しないエラーが発生しました。もう一度お試しください。</string> | |||
<string name="system">端末設定に従う</string> |
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.
🇯🇵
} | ||
|
||
@Composable | ||
fun Theme( |
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.
How about making it private and naming it ThemeSetting, as it can be mistaken for setting Theme? 🎨
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.
Thanks, I've changed it that way!
👍
Thank you! We will create issue! |
Sorry. Please fix conflict 🙏 |
@takahirom
I created an issue! |
Sorry, the CI is down, I'll contact you after I fix it. |
@takahirom |
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.
LGTM 👍 I think this is a good sample for changing the theme in app!
Issue
Overview (Required)
Problem
Links
Screenshot