feat(ui): Added theme profile to log-viewer #21448
Conversation
Signed-off-by: Surajyadav <[email protected]>
🔴 Preview Environment stopped on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
|
lgtm |
| interface LogViewerTheme { | ||
| light: boolean; | ||
| dark: boolean; | ||
| } |
There was a problem hiding this comment.
This data structure is strange to me. What would be the meaning of {light: true, dark: true}?
There was a problem hiding this comment.
I think the structure is really
interface UseDarkModeInLogViewerForAppTheme {
light: boolean;
dark: boolean;
}where light: false means "don't use dark mode in log viewer when in light mode app theme" and dark: true means "use dark mode in log viewer when in dark mode app theme".
and light: true means "use dark mode in log viewer when in light mode app theme" and dark: false means "use light mode in log viewer when in dark mode app theme".
Those last two settings exist to enable users to hurt their eyes, but apparently they're desirable modes, so here we are :)
-- I can't think of a particularly better way to structure the thing, but...
There was a problem hiding this comment.
yes, this is what it represent my naming wasn't quite right
| LogViewerTheme: { | ||
| light: false, | ||
| dark: true | ||
| }, | ||
| darkMode: currentTheme === 'dark' |
There was a problem hiding this comment.
Are we duplicating the theme params here?
There was a problem hiding this comment.
🤔 yes, there is duplication happening here as both darkMode and LogViewerTheme are controlling the state and used to update and store preferences,
tried multiple ways to have the userpreference with the current darkMode controller without duplication coudn't get to a good solution without it, I am refactoring it with small change like using darkMode itself
LogViewerTheme: {
light: prefs.appDetails.darkMode,
dark: prefs.appDetails.darkMode
}
and if this is getting overly complicated or redundant we can just have the log-viewer darkMode toggle controlled by global theme instead with no saved preferences. let me know where can I improve if this is not quite right
Signed-off-by: Surajyadav <[email protected]>
Signed-off-by: Surajyadav <[email protected]>
There was a problem hiding this comment.
LGTM, but the logic is quite hard to understand. This might be caused by the variable naming such as profile and newDarkMode.
The behavior in the video LGTM. But if I read the code, I have no idea that it is doing that. I think the code should be a bit more self-explaining.
Closes #21426
The log viewer will now be set using the preferred theme.
By default, it adopts the global theme as the log viewer profile unless the user has already set a specific preference for that mode
logview-profile.mp4
Checklist: