-
Notifications
You must be signed in to change notification settings - Fork 10
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
Viewer theme can be switched between dark or light theme #433
Conversation
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.
@ruchimotwaniBC , looks good, thanks!
Could you please change the implementation logic as follows:
- Change the control state's
currentAppTheme
type tostring
. - If
Config.instance.branding.themeName
is set, initialize control state'scurrentAppTheme
to its value. - Only if
Config.instance.branding.themeName
is NOT set, allow for changing the theme in the Settings.
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.
Reviewed from a user-perspective:
The switch between themes works really well.
But it seems to have no effect on the apprerance of an editor used in the following features:
- Import Places
- Sidebar -> Metadata -> JSON Format
Ok I check. Thanks @clarasb |
@@ -45,6 +45,7 @@ const mapStateToProps = (state: AppState) => { | |||
userVariables: selectedUserVariablesSelector(state), | |||
expressionCapabilities: expressionCapabilitiesSelector(state), | |||
serverUrl: selectedServerSelector(state).url, | |||
applicationTheme: state.controlState.applicationTheme, |
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.
The current theme should be available through the MUI theme context object. Please double-check the MUI docs on how to that (usually using the React ? useContext()` hook.
@@ -73,6 +74,7 @@ export default function UserVariablesDialog({ | |||
updateDatasetUserVariables, | |||
expressionCapabilities, | |||
serverUrl, | |||
applicationTheme, |
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.
Use context hook?
src/components/MarkdownPopover.tsx
Outdated
} | ||
|
||
export default function MarkdownPopover({ | ||
anchorEl, | ||
markdownText, | ||
open, | ||
onClose, | ||
applicationTheme, |
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.
Use hook.
src/components/HelpButton.tsx
Outdated
@@ -32,9 +32,10 @@ import MarkdownPopover from "@/components/MarkdownPopover"; | |||
interface HelpButtonProps { | |||
size?: "small" | "medium" | "large"; | |||
helpUrl?: string; | |||
applicationTheme?: 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.
Use hook.
@@ -79,7 +79,7 @@ export interface Branding { | |||
windowTitle: string; | |||
windowIcon: string | null; | |||
compact: boolean; | |||
themeName: "dark" | "light"; | |||
themeName: "light"; |
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.
A type with one choice? Why does this make sense?
The branding should provide the initial 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.
Will fix that.
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.
The feature looks great!
(tested from a user-perspective)
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 @ruchimotwaniBC
APPLICATION_THEMES, | ||
ApplicationThemes, |
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.
Why two of them?
@@ -79,7 +79,7 @@ export interface Branding { | |||
windowTitle: string; | |||
windowIcon: string | null; | |||
compact: boolean; | |||
themeName: "dark" | "light"; | |||
themeName: "light"; |
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.
Will fix that.
@@ -160,6 +166,7 @@ export interface ControlState { | |||
exportPlacesAsCollection: boolean; | |||
exportZipArchive: boolean; | |||
exportFileName: string; | |||
themeMode: 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.
"light" | "dark" | "system"
- I will fix that
Implemented the theme changing dropdown in Settings dialog.
The default theme of the application is taken from config.json and if not then default theme is according to users system.
Changes in MarkdownPopover are to support the link color for dark theme.