-
-
Notifications
You must be signed in to change notification settings - Fork 672
Prepare for "Auto - follow OS" option for Theme setting #5527
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
Changes from all commits
382556f
f6ff91a
3d7c169
33750f8
43acc41
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 |
---|---|---|
@@ -1,17 +1,17 @@ | ||
/* @flow strict-local */ | ||
import { getStatusBarColor } from '../ZulipStatusBar'; | ||
|
||
const themeNight = 'night'; | ||
const themeDefault = 'default'; | ||
const themeDark = 'dark'; | ||
const themeLight = 'light'; | ||
|
||
describe('getStatusBarColor', () => { | ||
test('returns specific color when given, regardless of theme', () => { | ||
expect(getStatusBarColor('#fff', themeDefault)).toEqual('#fff'); | ||
expect(getStatusBarColor('#fff', themeNight)).toEqual('#fff'); | ||
expect(getStatusBarColor('#fff', themeLight)).toEqual('#fff'); | ||
expect(getStatusBarColor('#fff', themeDark)).toEqual('#fff'); | ||
}); | ||
|
||
test('returns color according to theme for default case', () => { | ||
expect(getStatusBarColor(undefined, themeDefault)).toEqual('white'); | ||
expect(getStatusBarColor(undefined, themeNight)).toEqual('hsl(212, 28%, 18%)'); | ||
expect(getStatusBarColor(undefined, themeLight)).toEqual('white'); | ||
expect(getStatusBarColor(undefined, themeDark)).toEqual('hsl(212, 28%, 18%)'); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -348,10 +348,24 @@ export type RealmState = {| | |
+serverEmojiData: ServerEmojiData | null, | ||
|}; | ||
|
||
// TODO: Stop using the 'default' name. Any 'default' semantics should | ||
// only apply the device level, not within the app. See | ||
// https://github.com/zulip/zulip-mobile/issues/4009#issuecomment-619280681. | ||
export type ThemeName = 'default' | 'night'; | ||
/** | ||
* The visual theme of the app. | ||
* | ||
* To get a ThemeName from the ThemeSetting, first check the current | ||
* OS theme by calling the React Native hook useColorScheme and pass | ||
* that to the helper function getThemeToUse. | ||
Comment on lines
+354
to
+356
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. Cool, this is helpful information to include in jsdoc. |
||
*/ | ||
export type ThemeName = 'light' | 'dark'; | ||
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. commit-message nit:
The summary line should be capitalized after the colon-prefix, so like:
(I think the branch I provided for splitting the changes may have been confusing on this, because I had summary lines like 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. Thanks. I think I had this correct before but missed this detail after your slice and dice help. 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. Also in this commit message, a bit more substantive:
This commit doesn't fix #5169, because that's about what the user sees and this commit is internal to the code. The next commit does, though, so the "fixes" line should just go there. Also on the "fixes" line, a formatting nit: |
||
|
||
/** | ||
* The theme setting. | ||
* | ||
* This represents the value the user chooses in SettingsScreen. | ||
* | ||
* To determine the actual theme to show the user, use a ThemeName; | ||
* see there for details. | ||
*/ | ||
export type ThemeSetting = 'default' | 'night'; | ||
|
||
/** What browser the user has set to use for opening links in messages. | ||
* | ||
|
@@ -392,7 +406,7 @@ export type GlobalSettingsState = $ReadOnly<{ | |
// The user's chosen language, as an IETF BCP 47 language tag. | ||
language: string, | ||
|
||
theme: ThemeName, | ||
theme: ThemeSetting, | ||
browser: BrowserPreference, | ||
|
||
// TODO cut this? what was it? | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
/* @flow strict-local */ | ||
import type { ColorSchemeName } from 'react-native/Libraries/Utilities/NativeAppearance'; | ||
import type { ThemeSetting, ThemeName } from '../reduxTypes'; | ||
|
||
export const getThemeToUse = (theme: ThemeSetting, osScheme: ?ColorSchemeName): ThemeName => | ||
theme === 'default' ? 'light' : 'dark'; |
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.
Ah I see, in several of these components it won't suffice to say
useContext(ThemeProvider)
because the latter only gives theThemeData
(with the concrete colors) and not the identity of the theme as dark or light.As the existing comment here says, it'd be good to clean that up at some point. But for purposes of this PR, I'll be happy to let that be.
Still, let's avoid duplicating all those conditionals involved in computing
themeToUse
. These components can each call a central function, like thegetThemeToUse
described above, and let that take care of that logic.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.
Done.