-
Notifications
You must be signed in to change notification settings - Fork 404
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
Conversation
const theme = config?.theme | ||
if (theme && localStorageService.get(BrowserStorageItem.theme) !== theme) | ||
themeService.applyTheme(theme as Theme) | ||
changeTheme(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
|
||
const getQueryTheme = () => { | ||
const queryThemeParam = new URLSearchParams(window.location.search) | ||
.get('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.
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.
Code Coverage - Frontend unit tests
Test suite run success5162 tests passing in 677 suites. Report generated by 🧪jest coverage report action from be9103e |
Description
For RiC we want to get the theme from SM. The agreed upon approach is to pass it as query param. This PR sets up the handling of such query param. There will be another one for SM that will pass it.
The theme comes from:
Now, with this change, I'm adding a step inbetween 1. and 2., call it 1.5 - if theme query param is passed and it contains valid theme key value, that will be used over the default/localstorage value (updating the localstorage in the process). If
For RiC there's no sqlitdb config, the endpoint returns theme:null, hence the 3rd point above will not update the app theme.
For non-RiC (e.g. web/docker build) the query param will only override the default theme, then this state will be overridden by the /settings theme data, should one present. This is just explaining the behaviour, although it's not expected to use the theme query param for these builds.