PMM 13702 theme toggle button compatibility dark theme #4682
Conversation
dmitri-saricev-3pillargloball
requested review from
YashSartanpara1,
Copilot and
matejkubinec
|
|
There was a problem hiding this comment.
Pull Request Overview
This pull request implements theme toggle button compatibility for dark themes across the PMM UI. The changes establish bidirectional theme synchronization between the PMM UI and Grafana iframe, allowing theme changes to propagate correctly in both directions.
Key changes include:
- Addition of comprehensive theme synchronization mechanisms between PMM UI and Grafana
- Implementation of a theme toggle button in the navigation that dynamically updates based on current theme
- Enhanced Vite configuration for better development proxy support
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/docker-compose.yml | Updates image reference and enables custom Grafana code mounting |
| ui/apps/pmm/vite.config.ts | Extensive proxy configuration updates for development environment |
| ui/apps/pmm/tsconfig.node.json | Minor formatting update to include array |
| ui/apps/pmm/src/themes/setTheme.ts | New comprehensive theme management hook with local/remote sync |
| ui/apps/pmm/src/hooks/useGrafanaThemeSyncOnce.ts | New hook for one-time Grafana theme synchronization |
| ui/apps/pmm/src/hooks/theme.ts | Enhanced theme hook with Grafana synchronization |
| ui/apps/pmm/src/contexts/grafana/grafana.provider.tsx | Major updates to handle bidirectional theme messaging |
| ui/apps/pmm/src/components/sidebar/nav-item/NavItem.tsx | Theme toggle button implementation with dynamic labels/icons |
| ui/apps/pmm/src/App.tsx | Addition of theme sync guard component |
| ui/apps/pmm-compat/src/theme.ts | Enhanced theme management with postMessage bridge |
| ui/apps/pmm-compat/src/contexts/theme/theme.provider.tsx | Complete rewrite with DOM theme application and host notification |
| ui/apps/pmm-compat/src/compat.ts | Integration of theme wiring setup |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
dmitri-saricev-3pillargloball
force-pushed
the
PMM-13702-theme-toggle-button-compatibility-dark-theme
branch
from
81a1d59 to
50190e6
Compare
| const scheme = mode === 'dark' ? 'percona-dark' : 'percona-light'; | ||
|
|
||
| // Set attributes our CSS reads | ||
| html.setAttribute('data-md-color-scheme', scheme); |
There was a problem hiding this comment.
Why do we need to modify the HTML attributes?
| // Notify outer PMM UI (new nav) to sync immediately | ||
| try { | ||
| const target = window.top && window.top !== window ? window.top : window.parent || window; | ||
| target?.postMessage({ type: 'grafana.theme.changed', payload: { mode } }, targetOrigin); |
There was a problem hiding this comment.
Use messenger to handle the communication between the iframe and pmm-ui (no need to handle the target, etc.). We also use predefined message types (see ui/packages/shared/src/types.ts)
|
|
||
| type Mode = 'light' | 'dark'; | ||
|
|
||
| export const ThemeProvider: FC<PropsWithChildren> = ({ children }) => { |
There was a problem hiding this comment.
I don't think you need to touch this provider at all, I think I'll add a comment there, but this specifically is used just for two toolbar buttons used inside the grafana iframe where we extend the functionality. See ui/apps/pmm-compat/src/compat/toolbar.tsx for usage, but in short we append two divs which we then take as root for two small react apps (just a button for each)
| * --------------------------*/ | ||
|
|
||
| // Normalize and apply <html> attributes so CSS-based nav updates immediately | ||
| function applyHtmlTheme(modeRaw: unknown) { |
There was a problem hiding this comment.
Why is there a need to apply the theme to html? The changeTheme handles the theme change on the grafana side.
| } | ||
|
|
||
| /** Dev-only handshake: lock '*' to the real origin after the first ACK. */ | ||
| (function setupOriginHandshake() { |
There was a problem hiding this comment.
Handshake is pretty good idea to not send information where we do not want.
| type Mode = 'light' | 'dark'; | ||
|
|
||
| /** Normalizes any incoming value to 'light' | 'dark'. */ | ||
| function normalizeMode(v: unknown): Mode { |
There was a problem hiding this comment.
Do we expect mode to be something other than light/dark? If we need normalization why are there like 3 different implementations?
| * - persistence to Grafana preferences | ||
| * - broadcasting to Grafana iframe (right) | ||
| */ | ||
| export function useSetTheme() { |
There was a problem hiding this comment.
It would be good to centralize the theme handling/changes into a separate provider.
Should load the current theme based on the localStorage/userPreferences and store changes in user preferences when theme changes.
| // 2) persist to Grafana (only for left-initiated actions) | ||
| if (opts.persist) { | ||
| try { | ||
| await grafanaApi.put('/user/preferences', { theme: next }); |
There was a problem hiding this comment.
what's the reason behind using api directly instead of through react query?
| }, | ||
| "include": ["vite.config.ts"] | ||
| "include": [ | ||
| "./vite.config.ts" |
There was a problem hiding this comment.
Do we need this change?
| @@ -0,0 +1,47 @@ | |||
| /** | |||
There was a problem hiding this comment.
Why do we need a specific vitest config?
3 participants
PMM-0
Link to the Feature Build: SUBMODULES-0
If this PR adds or removes or alters one or more API endpoints, please review and add or update the relevant API documents as well:
If this PR is related to some other PRs in this or other repositories, please provide links to those PRs:
Links to related pull requests (optional).
FB
-PMM-13702: Theme toggle button compatibility dark theme Percona-Lab/pmm-submodules#4083