From 808b3712254854a5946d4d75ce8fa7a4a901885d Mon Sep 17 00:00:00 2001
From: Ashley Harrison <ashley.harrison@grafana.com>
Date: Mon, 19 May 2025 12:42:36 +0100
Subject: [PATCH] Themes: Prevent duplicated API call in drawer (#105602)

refactor to separate component, stop propagating radio button event + unit test
---
 .../ThemeSelector/ThemeCard.test.tsx          | 35 ++++++++
 .../components/ThemeSelector/ThemeCard.tsx    | 84 +++++++++++++++++++
 .../ThemeSelector/ThemeSelectorDrawer.tsx     | 75 +----------------
 3 files changed, 122 insertions(+), 72 deletions(-)
 create mode 100644 public/app/core/components/ThemeSelector/ThemeCard.test.tsx
 create mode 100644 public/app/core/components/ThemeSelector/ThemeCard.tsx

diff --git a/public/app/core/components/ThemeSelector/ThemeCard.test.tsx b/public/app/core/components/ThemeSelector/ThemeCard.test.tsx
new file mode 100644
index 00000000000..050610d373a
--- /dev/null
+++ b/public/app/core/components/ThemeSelector/ThemeCard.test.tsx
@@ -0,0 +1,35 @@
+import { render, screen } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+
+import { createTheme, ThemeRegistryItem } from '@grafana/data';
+
+import { ThemeCard } from './ThemeCard';
+
+describe('ThemeCard', () => {
+  let user: ReturnType<typeof userEvent.setup>;
+
+  beforeEach(() => {
+    user = userEvent.setup();
+  });
+
+  const mockTheme: ThemeRegistryItem = {
+    id: 'dark',
+    name: 'Dark',
+    build: createTheme,
+  };
+
+  it('should only call onSelect once when clicking the radio button dot', async () => {
+    const onSelectMock = jest.fn();
+
+    render(<ThemeCard themeOption={mockTheme} onSelect={onSelectMock} isSelected={false} />);
+
+    // Find the radio button input element
+    const radioButtonInput = screen.getByRole('radio');
+
+    // Click the radio button
+    await user.click(radioButtonInput);
+
+    // Check that onSelect was called only once
+    expect(onSelectMock).toHaveBeenCalledTimes(1);
+  });
+});
diff --git a/public/app/core/components/ThemeSelector/ThemeCard.tsx b/public/app/core/components/ThemeSelector/ThemeCard.tsx
new file mode 100644
index 00000000000..38892d8fa30
--- /dev/null
+++ b/public/app/core/components/ThemeSelector/ThemeCard.tsx
@@ -0,0 +1,84 @@
+import { css } from '@emotion/css';
+
+import { FeatureState, GrafanaTheme2, ThemeRegistryItem } from '@grafana/data';
+import { useTranslate } from '@grafana/i18n';
+import { TFunction } from '@grafana/i18n/internal';
+import { FeatureBadge, RadioButtonDot, useStyles2 } from '@grafana/ui';
+
+import { ThemePreview } from '../Theme/ThemePreview';
+
+interface ThemeCardProps {
+  themeOption: ThemeRegistryItem;
+  isExperimental?: boolean;
+  isSelected?: boolean;
+  onSelect: () => void;
+}
+
+export function ThemeCard({ themeOption, isExperimental, isSelected, onSelect }: ThemeCardProps) {
+  const { t } = useTranslate();
+  const theme = themeOption.build();
+  const label = getTranslatedThemeName(themeOption, t);
+  const styles = useStyles2(getStyles);
+
+  return (
+    // this is a convenience for mouse users. keyboard/screen reader users will use the radio button
+    // eslint-disable-next-line jsx-a11y/no-static-element-interactions,jsx-a11y/click-events-have-key-events
+    <div className={styles.card} onClick={onSelect}>
+      <div className={styles.header}>
+        <RadioButtonDot
+          id={`theme-${theme.name}`}
+          name={'theme'}
+          label={label}
+          onClick={(event) => {
+            // prevent propagation so that onSelect is only called once when clicking the radio button
+            event.stopPropagation();
+          }}
+          onChange={onSelect}
+          checked={isSelected}
+        />
+        {isExperimental && <FeatureBadge featureState={FeatureState.experimental} />}
+      </div>
+      <ThemePreview theme={theme} />
+    </div>
+  );
+}
+
+const getStyles = (theme: GrafanaTheme2) => {
+  return {
+    card: css({
+      border: `1px solid ${theme.colors.border.weak}`,
+      borderRadius: theme.shape.radius.default,
+      display: 'flex',
+      flexDirection: 'column',
+      overflow: 'hidden',
+      cursor: 'pointer',
+      '&:hover': {
+        border: `1px solid ${theme.colors.border.medium}`,
+      },
+    }),
+    header: css({
+      alignItems: 'center',
+      borderBottom: `1px solid ${theme.colors.border.weak}`,
+      display: 'flex',
+      justifyContent: 'space-between',
+      padding: theme.spacing(1),
+      // The RadioButtonDot is not correctly implemented at the moment, missing cursor (And click ability for the label and input)
+      '> label': {
+        cursor: 'pointer',
+      },
+    }),
+  };
+};
+
+function getTranslatedThemeName(theme: ThemeRegistryItem, t: TFunction) {
+  switch (theme.id) {
+    case 'dark':
+      return t('shared.preferences.theme.dark-label', 'Dark');
+    case 'light':
+      return t('shared.preferences.theme.light-label', 'Light');
+    case 'system':
+      return t('shared.preferences.theme.system-label', 'System preference');
+    default:
+      return theme.name;
+  }
+}
diff --git a/public/app/core/components/ThemeSelector/ThemeSelectorDrawer.tsx b/public/app/core/components/ThemeSelector/ThemeSelectorDrawer.tsx
index 6f8be9d6972..81517c86649 100644
--- a/public/app/core/components/ThemeSelector/ThemeSelectorDrawer.tsx
+++ b/public/app/core/components/ThemeSelector/ThemeSelectorDrawer.tsx
@@ -1,14 +1,12 @@
 import { css } from '@emotion/css';
 
-import { FeatureState, GrafanaTheme2, ThemeRegistryItem } from '@grafana/data';
+import { GrafanaTheme2, ThemeRegistryItem } from '@grafana/data';
 import { Trans, useTranslate } from '@grafana/i18n';
-import { TFunction } from '@grafana/i18n/internal';
 import { config, reportInteraction } from '@grafana/runtime';
-import { Drawer, FeatureBadge, RadioButtonDot, TextLink, useStyles2, useTheme2 } from '@grafana/ui';
+import { Drawer, TextLink, useStyles2, useTheme2 } from '@grafana/ui';
 import { changeTheme } from 'app/core/services/theme';
 
-import { ThemePreview } from '../Theme/ThemePreview';
-
+import { ThemeCard } from './ThemeCard';
 import { getSelectableThemes } from './getSelectableThemes';
 
 interface Props {
@@ -63,38 +61,6 @@ export function ThemeSelectorDrawer({ onClose }: Props) {
   );
 }
 
-interface ThemeCardProps {
-  themeOption: ThemeRegistryItem;
-  isExperimental?: boolean;
-  isSelected?: boolean;
-  onSelect: () => void;
-}
-
-function ThemeCard({ themeOption, isExperimental, isSelected, onSelect }: ThemeCardProps) {
-  const { t } = useTranslate();
-  const theme = themeOption.build();
-  const label = getTranslatedThemeName(themeOption, t);
-  const styles = useStyles2(getStyles);
-
-  return (
-    // this is a convenience for mouse users. keyboard/screen reader users will use the radio button
-    // eslint-disable-next-line jsx-a11y/no-static-element-interactions,jsx-a11y/click-events-have-key-events
-    <div className={styles.card} onClick={onSelect}>
-      <div className={styles.header}>
-        <RadioButtonDot
-          id={`theme-${theme.name}`}
-          name={'theme'}
-          label={label}
-          onChange={onSelect}
-          checked={isSelected}
-        />
-        {isExperimental && <FeatureBadge featureState={FeatureState.experimental} />}
-      </div>
-      <ThemePreview theme={theme} />
-    </div>
-  );
-}
-
 const getStyles = (theme: GrafanaTheme2) => {
   return {
     grid: css({
@@ -103,40 +69,5 @@ const getStyles = (theme: GrafanaTheme2) => {
       gridAutoRows: `250px`,
       gap: theme.spacing(2),
     }),
-    card: css({
-      border: `1px solid ${theme.colors.border.weak}`,
-      borderRadius: theme.shape.radius.default,
-      display: 'flex',
-      flexDirection: 'column',
-      overflow: 'hidden',
-      cursor: 'pointer',
-      '&:hover': {
-        border: `1px solid ${theme.colors.border.medium}`,
-      },
-    }),
-    header: css({
-      alignItems: 'center',
-      borderBottom: `1px solid ${theme.colors.border.weak}`,
-      display: 'flex',
-      justifyContent: 'space-between',
-      padding: theme.spacing(1),
-      // The RadioButtonDot is not correctly implemented at the moment, missing cursor (And click ability for the label and input)
-      '> label': {
-        cursor: 'pointer',
-      },
-    }),
   };
 };
-
-function getTranslatedThemeName(theme: ThemeRegistryItem, t: TFunction) {
-  switch (theme.id) {
-    case 'dark':
-      return t('shared.preferences.theme.dark-label', 'Dark');
-    case 'light':
-      return t('shared.preferences.theme.light-label', 'Light');
-    case 'system':
-      return t('shared.preferences.theme.system-label', 'System preference');
-    default:
-      return theme.name;
-  }
-}