-
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 all 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 @@ | |
|
||
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,14 +49,20 @@ | |
constructor(props: any) { | ||
super(props) | ||
|
||
const queryTheme = getQueryTheme() | ||
const storedThemeValue = localStorageService.get(BrowserStorageItem.theme) | ||
const theme = | ||
!storedThemeValue || !THEME_NAMES.includes(storedThemeValue) | ||
? defaultState.theme | ||
: storedThemeValue | ||
|
||
let theme = defaultState.theme | ||
|
||
if (queryTheme) { | ||
theme = queryTheme | ||
} else if (storedThemeValue && THEME_NAMES.includes(storedThemeValue)) { | ||
theme = storedThemeValue | ||
} | ||
Check warning on line 61 in redisinsight/ui/src/contexts/themeContext.tsx
|
||
|
||
const usingSystemTheme = theme === Theme.System | ||
|
||
themeService.applyTheme(theme) | ||
themeService.applyTheme(theme as Theme) | ||
|
||
this.state = { | ||
theme: theme === Theme.System ? this.getSystemTheme() : 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