-
Notifications
You must be signed in to change notification settings - Fork 407
RI-7574: allow setting default theme from query param #5031
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 2 commits
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 |
---|---|---|
|
@@ -26,6 +26,14 @@ interface Props { | |
|
||
const THEME_NAMES = THEMES.map(({ value }) => value) | ||
|
||
const getQueryTheme = () => { | ||
const queryThemeParam = new URLSearchParams(window.location.search) | ||
pd-redis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.get('theme') | ||
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. Note: It's not directly related to your changes, but I would suggest putting this "magic string" behind an enum. We have similar logic in about 15 other places in the codebase where we use the same approach. I believe having a centralized location where we can view a list of all the available query parameters that Redis Insight accepts and handles will be beneficial. It will be both cleaner and also better in terms of self-explanatory "code documentation". If you find this advice useful, we can log a separate tech debt ticket with low priority and reference it outside the scope of this change. |
||
?.toUpperCase() | ||
|
||
return THEMES.find(({ value }) => value === queryThemeParam)?.value | ||
} | ||
|
||
export const defaultState = { | ||
theme: DEFAULT_THEME || Theme.System, | ||
usingSystemTheme: | ||
|
@@ -41,11 +49,14 @@ export class ThemeProvider extends React.Component<Props> { | |
constructor(props: any) { | ||
super(props) | ||
|
||
// theme query param can be used to override the local/default theme | ||
const queryTheme = getQueryTheme() | ||
const storedThemeValue = localStorageService.get(BrowserStorageItem.theme) | ||
const theme = | ||
!storedThemeValue || !THEME_NAMES.includes(storedThemeValue) | ||
queryTheme || | ||
KrumTy marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
(!storedThemeValue || !THEME_NAMES.includes(storedThemeValue) | ||
? defaultState.theme | ||
: storedThemeValue | ||
: storedThemeValue) | ||
const usingSystemTheme = theme === Theme.System | ||
|
||
themeService.applyTheme(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.
changing theme via changeTheme is the correct approach as it updates the theme provider and calls themeService.applyTheme internally