DynamicDashboards: Make dock/undock more intuitive (#119931)

e2e-playwright/dashboard-new-layouts/dashboards-add-panel.spec.ts

@@ -41,7 +41,7 @@ test.describe(
       // Check that pressing the configure button shows the panel editor
       await dashboardPage
         .getByGrafanaSelector(selectors.components.Panels.Panel.content)
-        .filter({ hasText: 'Configure' })
+        .getByRole('button', { name: /configure/i })
         .click();
       await expect(dashboardPage.getByGrafanaSelector(selectors.components.PanelEditor.General.content)).toBeVisible();
     });

packages/grafana-ui/src/components/Sidebar/Sidebar.tsx

@@ -78,14 +78,12 @@ export function SiderbarToolbar({ children }: SiderbarToolbarProps) {
     <div className={cx(styles.toolbar, context.compact && styles.toolbarIconsOnly)}>
       {children}
       <div className={styles.flexGrow} />
-      {context.hasOpenPane && (
-        <SidebarButton
-          icon={'web-section-alt'}
-          onClick={context.onToggleDock}
-          title={context.isDocked ? t('grafana-ui.sidebar.undock', 'Undock') : t('grafana-ui.sidebar.dock', 'Dock')}
-          data-testid={selectors.components.Sidebar.dockToggle}
-        />
-      )}
+      <SidebarButton
+        icon={'web-section-alt'}
+        onClick={context.onToggleDock}
+        title={context.isDocked ? t('grafana-ui.sidebar.undock', 'Undock') : t('grafana-ui.sidebar.dock', 'Dock')}
+        data-testid={selectors.components.Sidebar.dockToggle}
+      />
     </div>
   );
 }

packages/grafana-ui/src/components/Sidebar/useSidebar.test.tsx

@@ -0,0 +1,113 @@
+import { renderHook } from '@testing-library/react';
+
+import { store } from '@grafana/data';
+
+import { useSidebarSavedState } from './useSidebar';
+
+jest.mock('@grafana/data', () => ({
+  ...jest.requireActual('@grafana/data'),
+  store: {
+    get: jest.fn(),
+    set: jest.fn(),
+    getBool: jest.fn(),
+  },
+}));
+
+const mockedStore = jest.mocked(store, { shallow: true });
+
+describe('useSidebarSavedState(persistanceKey, subKey, defaultValue)', () => {
+  beforeEach(() => {
+    jest.clearAllMocks();
+  });
+
+  describe('when persistanceKey is not passed', () => {
+    test('returns the default value', () => {
+      const { result } = renderHook(() => useSidebarSavedState(undefined, 'subKey1', 1));
+
+      const [state] = result.current;
+      expect(state).toBe(1);
+    });
+  });
+
+  describe('if defaultValue is a boolean', () => {
+    test('reads a boolean value from the store', () => {
+      mockedStore.getBool.mockReturnValue(true);
+
+      const [persistanceKey, subKey, defaultValue] = ['persistanceKey2', 'subKey2', false];
+      const { result } = renderHook(() => useSidebarSavedState(persistanceKey, subKey, defaultValue));
+
+      expect(mockedStore.getBool).toHaveBeenCalledWith(`grafana.ui.sidebar.${persistanceKey}.${subKey}`, defaultValue);
+
+      const [state] = result.current;
+      expect(state).toBe(true);
+    });
+  });
+
+  describe('if defaultValue is a number', () => {
+    test('converts the value read from the store to an integer', () => {
+      mockedStore.get.mockReturnValue('7');
+
+      const [persistanceKey, subKey, defaultValue] = ['persistanceKey3', 'subKey3', 3];
+      const { result } = renderHook(() => useSidebarSavedState(persistanceKey, subKey, defaultValue));
+
+      expect(mockedStore.get).toHaveBeenCalledWith(`grafana.ui.sidebar.${persistanceKey}.${subKey}`);
+
+      const [state] = result.current;
+      expect(state).toBe(7);
+    });
+
+    test('returns defaultValue when the conversion fails', () => {
+      mockedStore.get.mockReturnValue('seven');
+
+      const [persistanceKey, subKey, defaultValue] = ['persistanceKey4', 'subKey4', 4];
+      const { result } = renderHook(() => useSidebarSavedState(persistanceKey, subKey, defaultValue));
+
+      expect(mockedStore.get).toHaveBeenCalledWith(`grafana.ui.sidebar.${persistanceKey}.${subKey}`);
+
+      const [state] = result.current;
+      expect(state).toBe(4);
+    });
+  });
+
+  describe('if defaultValue is neither a boolean nor a number', () => {
+    test('returns the default value', () => {
+      const [persistanceKey, subKey, defaultValue] = ['persistanceKey5', 'subKey5', { test: 'five' }];
+      const { result } = renderHook(() => useSidebarSavedState(persistanceKey, subKey, defaultValue));
+
+      expect(mockedStore.get).not.toHaveBeenCalled();
+
+      const [state] = result.current;
+      expect(state).toBe(defaultValue);
+    });
+  });
+
+  describe('when persistanceKey or subKey changes across re-renders', () => {
+    test('re-reads the value from the store using the new key', () => {
+      mockedStore.get.mockImplementation((key: string) => {
+        const storeValues: Record<string, string> = {
+          'grafana.ui.sidebar.persistanceKey6.subKeyA': '10',
+          'grafana.ui.sidebar.persistanceKey7.subKeyA': '20',
+          'grafana.ui.sidebar.persistanceKey7.subKeyB': '30',
+        };
+        return storeValues[key];
+      });
+
+      const initialProps = { persistanceKey: 'persistanceKey6', subKey: 'subKeyA', defaultValue: 42 };
+
+      const { result, rerender } = renderHook(
+        (props) => useSidebarSavedState(props.persistanceKey, props.subKey, props.defaultValue),
+        { initialProps }
+      );
+
+      expect(result.current[0]).toBe(10);
+
+      rerender({ ...initialProps, persistanceKey: 'persistanceKey7' });
+
+      expect(result.current[0]).toBe(20);
+
+      rerender({ ...initialProps, persistanceKey: 'persistanceKey7', subKey: 'subKeyB' });
+
+      expect(result.current[0]).toBe(30);
+    });
+  });
+});

packages/grafana-ui/src/components/Sidebar/useSidebar.tsx

@@ -1,5 +1,5 @@
 import { clamp } from 'lodash';
-import React, { useCallback } from 'react';
+import React, { useCallback, useEffect } from 'react';
 
 import { store } from '@grafana/data';
 
@@ -144,33 +144,38 @@ export function useSidebar({
   };
 }
 
-function useSidebarSavedState<T = number | boolean>(
+function readFromStore<T>(persistanceKey: string | undefined, subKey: string, defaultValue: T): T {
+  if (!persistanceKey) {
+    return defaultValue;
+  }
+
+  if (typeof defaultValue === 'boolean') {
+    // eslint-disable-next-line @typescript-eslint/consistent-type-assertions
+    return store.getBool(`grafana.ui.sidebar.${persistanceKey}.${subKey}`, defaultValue) as T;
+  }
+
+  if (typeof defaultValue === 'number') {
+    const value = Number.parseInt(store.get(`grafana.ui.sidebar.${persistanceKey}.${subKey}`), 10);
+    // eslint-disable-next-line @typescript-eslint/consistent-type-assertions
+    return Number.isNaN(value) ? defaultValue : (value as T);
+  }
+
+  return defaultValue;
+}
+
+export function useSidebarSavedState<T = number | boolean>(
   persistanceKey: string | undefined,
   subKey: string,
   defaultValue: T
 ) {
-  const [state, setState] = React.useState<T>(() => {
-    if (!persistanceKey) {
-      return defaultValue;
-    }
+  const [state, setState] = React.useState<T>(() => readFromStore(persistanceKey, subKey, defaultValue));
 
-    if (typeof defaultValue === 'boolean') {
-      // eslint-disable-next-line @typescript-eslint/consistent-type-assertions
-      return store.getBool(`grafana.ui.sidebar.${persistanceKey}.${subKey}`, defaultValue) as T;
-    }
-
-    if (typeof defaultValue === 'number') {
-      const value = Number.parseInt(store.get(`grafana.ui.sidebar.${persistanceKey}.${subKey}`), 10);
-      if (Number.isNaN(value)) {
-        return defaultValue;
-      }
-
-      // eslint-disable-next-line @typescript-eslint/consistent-type-assertions
-      return value as T;
-    }
-
-    return defaultValue;
-  });
+  useEffect(() => {
+    setState(readFromStore(persistanceKey, subKey, defaultValue));
+    // Re-read from storage when the persistence key changes, but not when defaultValue changes
+    // to avoid overriding a user-persisted value.
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [persistanceKey, subKey]);
 
   const setPersisted = useCallback(
     (cb: (prevState: T) => T) => {

public/app/features/dashboard-scene/edit-pane/DashboardEditPaneSplitter.tsx

@@ -80,7 +80,8 @@ function DashboardEditPaneSplitterNewLayouts({ dashboard, isEditing, body, contr
     hasOpenPane: Boolean(openPane),
     contentMargin: 1,
     position: 'right',
-    persistanceKey: 'dashboard',
+    persistanceKey: isEditing ? 'dashboard' : 'dashboard-view',
+    defaultToDocked: isEditing ? true : false,
     onClosePane: () => editPane.closePane(),
   });