From 98d182194867402f1233f01a7c5e9585c75dafa8 Mon Sep 17 00:00:00 2001 From: Kevin Adam <16607163+kevelopment@users.noreply.github.com> Date: Wed, 28 May 2025 16:39:23 +0200 Subject: [PATCH] Extension sidebar: Fix incorrect rendering of menu buttons (#106145) * fix(ui): use component render instead of usecallback to render button * add and fix component tests --- .../ExtensionSidebar/ExtensionToolbarItem.tsx | 106 +++++------------- .../ExtensionToolbarItemButton.test.tsx | 57 ++++++++++ .../ExtensionToolbarItemButton.tsx | 79 +++++++++++++ 3 files changed, 161 insertions(+), 81 deletions(-) create mode 100644 public/app/core/components/AppChrome/ExtensionSidebar/ExtensionToolbarItemButton.test.tsx create mode 100644 public/app/core/components/AppChrome/ExtensionSidebar/ExtensionToolbarItemButton.tsx diff --git a/public/app/core/components/AppChrome/ExtensionSidebar/ExtensionToolbarItem.tsx b/public/app/core/components/AppChrome/ExtensionSidebar/ExtensionToolbarItem.tsx index 10144d70031..7f9cfe74152 100644 --- a/public/app/core/components/AppChrome/ExtensionSidebar/ExtensionToolbarItem.tsx +++ b/public/app/core/components/AppChrome/ExtensionSidebar/ExtensionToolbarItem.tsx @@ -1,9 +1,4 @@ -import { css, cx } from '@emotion/css'; -import { useCallback } from 'react'; - -import { GrafanaTheme2 } from '@grafana/data'; -import { useTranslate } from '@grafana/i18n'; -import { Dropdown, Menu, ToolbarButton, useTheme2 } from '@grafana/ui'; +import { Dropdown, Menu } from '@grafana/ui'; import { NavToolbarSeparator } from '../NavToolbar/NavToolbarSeparator'; @@ -12,12 +7,11 @@ import { getComponentMetaFromComponentId, useExtensionSidebarContext, } from './ExtensionSidebarProvider'; +import { ExtensionToolbarItemButton } from './ExtensionToolbarItemButton'; export function ExtensionToolbarItem() { - const styles = getStyles(useTheme2()); const { availableComponents, dockedComponentId, setDockedComponentId, isOpen, isEnabled } = useExtensionSidebarContext(); - const { t } = useTranslate(); let dockedComponentTitle = ''; if (dockedComponentId) { @@ -40,52 +34,20 @@ export function ExtensionToolbarItem() { return null; } - // conditionally renders a button to open or close the sidebar - // not using a component to avoid passing refs with the `Dropdown` component - const renderButton = useCallback( - (isOpen: boolean, title?: string, onClick?: () => void) => { - if (isOpen) { - // render button to close the sidebar - return ( - setDockedComponentId(undefined)} - tooltip={t('navigation.extension-sidebar.button-tooltip.close', 'Close {{title}}', { title })} - /> - ); - } - // if a title is provided, use it in the tooltip - let tooltip = t('navigation.extension-sidebar.button-tooltip.open-all', 'Open AI assistants and sidebar apps'); - if (title) { - tooltip = t('navigation.extension-sidebar.button-tooltip.open', 'Open {{title}}', { title }); - } - return ( - - ); - }, - [setDockedComponentId, styles.button, styles.buttonActive, t] - ); - if (components.length === 1) { return ( <> - {renderButton(isOpen, components[0].title, () => { - if (isOpen) { - setDockedComponentId(undefined); - } else { - setDockedComponentId(getComponentIdFromComponentMeta(components[0].pluginId, components[0])); - } - })} + { + if (isOpen) { + setDockedComponentId(undefined); + } else { + setDockedComponentId(getComponentIdFromComponentMeta(components[0].pluginId, components[0])); + } + }} + /> ); @@ -114,40 +76,22 @@ export function ExtensionToolbarItem() { ); return ( <> - {isOpen && - renderButton(isOpen, dockedComponentTitle, () => { - if (isOpen) { - setDockedComponentId(undefined); - } - })} - {!isOpen && ( + {isOpen ? ( + { + if (isOpen) { + setDockedComponentId(undefined); + } + }} + /> + ) : ( - {renderButton(isOpen)} + )} ); } - -function getStyles(theme: GrafanaTheme2) { - return { - button: css({ - // this is needed because with certain breakpoints the button will get `width: auto` - // and the icon will stretch - aspectRatio: '1 / 1 !important', - width: '28px', - height: '28px', - padding: 0, - justifyContent: 'center', - borderRadius: theme.shape.radius.circle, - margin: theme.spacing(0, 0.25), - }), - buttonActive: css({ - borderRadius: theme.shape.radius.circle, - backgroundColor: theme.colors.primary.transparent, - border: `1px solid ${theme.colors.primary.borderTransparent}`, - color: theme.colors.text.primary, - }), - }; -} diff --git a/public/app/core/components/AppChrome/ExtensionSidebar/ExtensionToolbarItemButton.test.tsx b/public/app/core/components/AppChrome/ExtensionSidebar/ExtensionToolbarItemButton.test.tsx new file mode 100644 index 00000000000..3756924f507 --- /dev/null +++ b/public/app/core/components/AppChrome/ExtensionSidebar/ExtensionToolbarItemButton.test.tsx @@ -0,0 +1,57 @@ +import { render, screen, fireEvent } from '@testing-library/react'; + +import { useTranslate } from '@grafana/i18n'; + +import { ExtensionToolbarItemButton } from './ExtensionToolbarItemButton'; + +// Mock the useTranslate hook +jest.mock('@grafana/i18n', () => ({ + useTranslate: jest.fn(), +})); + +describe('ExtensionToolbarItemButton', () => { + const mockTranslate = (_: string, fallback: string, values?: Record) => { + if (values) { + return fallback.replace('{{title}}', values.title); + } + return fallback; + }; + + beforeEach(() => { + (useTranslate as jest.Mock).mockReturnValue({ t: mockTranslate }); + }); + + it('renders open button with default tooltip when no title is provided', () => { + render(); + + const button = screen.getByTestId('extension-toolbar-button-open'); + expect(button).toBeInTheDocument(); + expect(button).toHaveAttribute('aria-label', 'Open AI assistants and sidebar apps'); + }); + + it('renders open button with custom tooltip when title is provided', () => { + render(); + + const button = screen.getByTestId('extension-toolbar-button-open'); + expect(button).toBeInTheDocument(); + expect(button).toHaveAttribute('aria-label', 'Open Test App'); + }); + + it('renders close button with custom tooltip when isOpen is true', () => { + render(); + + const button = screen.getByTestId('extension-toolbar-button-close'); + expect(button).toBeInTheDocument(); + expect(button).toHaveAttribute('aria-label', 'Close Test App'); + }); + + it('calls onClick handler when button is clicked', () => { + const handleClick = jest.fn(); + render(); + + const button = screen.getByTestId('extension-toolbar-button-open'); + fireEvent.click(button); + + expect(handleClick).toHaveBeenCalledTimes(1); + }); +}); diff --git a/public/app/core/components/AppChrome/ExtensionSidebar/ExtensionToolbarItemButton.tsx b/public/app/core/components/AppChrome/ExtensionSidebar/ExtensionToolbarItemButton.tsx new file mode 100644 index 00000000000..070698d4190 --- /dev/null +++ b/public/app/core/components/AppChrome/ExtensionSidebar/ExtensionToolbarItemButton.tsx @@ -0,0 +1,79 @@ +import { css, cx } from '@emotion/css'; +import React from 'react'; + +import { GrafanaTheme2 } from '@grafana/data'; +import { useTranslate } from '@grafana/i18n'; +import { ToolbarButton, useStyles2 } from '@grafana/ui'; + +interface ToolbarItemButtonProps { + isOpen: boolean; + title?: string; + onClick?: () => void; +} + +function ExtensionToolbarItemButtonComponent( + { isOpen, title, onClick }: ToolbarItemButtonProps, + ref: React.ForwardedRef +) { + const styles = useStyles2(getStyles); + const { t } = useTranslate(); + + if (isOpen) { + // render button to close the sidebar + return ( + + ); + } + // if a title is provided, use it in the tooltip + let tooltip = t('navigation.extension-sidebar.button-tooltip.open-all', 'Open AI assistants and sidebar apps'); + if (title) { + tooltip = t('navigation.extension-sidebar.button-tooltip.open', 'Open {{title}}', { title }); + } + return ( + + ); +} + +// Wrapped the component with React.forwardRef to enable ref forwarding, which is required +// for proper integration with the Dropdown component from @grafana/ui +export const ExtensionToolbarItemButton = React.forwardRef( + ExtensionToolbarItemButtonComponent +); + +function getStyles(theme: GrafanaTheme2) { + return { + button: css({ + // this is needed because with certain breakpoints the button will get `width: auto` + // and the icon will stretch + aspectRatio: '1 / 1 !important', + width: '28px', + height: '28px', + padding: 0, + justifyContent: 'center', + borderRadius: theme.shape.radius.circle, + margin: theme.spacing(0, 0.25), + }), + buttonActive: css({ + borderRadius: theme.shape.radius.circle, + backgroundColor: theme.colors.primary.transparent, + border: `1px solid ${theme.colors.primary.borderTransparent}`, + color: theme.colors.text.primary, + }), + }; +}