GrafanaUI: Define tooltip or aria-label as required for IconButton (#…

…69699) * refactor: modify interfaces to make tooltip or aria-label required * refactor: change functionality around aria-label and tooltip * refactor: change and add information in storybook documentation * refactor: remove default from tooltip * refactor: IconButton to make tooltip or aria-label required * refactor: Fix tests * refactor: Fix tests * refactor: Fix tests * refactor: Fix tests * feat: add migration guide for breaking change * feat: add latest requirements to storybook docs * refactor: separate iconbutton story with and without tooltip * refactor: remove exported baseArgs * refactor: clean up and restructure original story * refactor: adjust styling * refactor: enable control for tooltip * refactor: clean up * refactor: enable control for aria-label * refactor: fix theme getting the wrong theme * refactor: fix tests * refactor: adjust story * refactor: remove confusing story * refactor: adjust controls for stories
grafana · Jun 26, 2023 · 11e96cc · 11e96cc
1 parent 72cde7c
commit 11e96cc
Show file tree

Hide file tree

Showing 22 changed files with 187 additions and 119 deletions.
diff --git a/docs/sources/developers/plugins/migration-guide/v10.0.x-v10.1.x/_index.md b/docs/sources/developers/plugins/migration-guide/v10.0.x-v10.1.x/_index.md
@@ -0,0 +1,29 @@
+---
+description: Guide for migrating plugins from Grafana v10.0.x to v10.1.x
+keywords:
+  - grafana
+  - plugins
+  - migration
+  - plugin
+  - documentation
+title: Migrate plugins from Grafana 10.0.x to 10.1.x
+menutitle: v10.0.x to v10.1.x
+weight: 1900
+---
+
+# Migrate plugins from Grafana version 10.0.x to 10.1.x
+
+## Accessibility update for IconButton component in grafana-ui
+
+We updated the component's TypeScript interface due to an accessibility issue. This change was delivered to the core `grafana` repo with [PR 69699](https://github.com/grafana/grafana/pull/69699).
+
+In case you are using the IconButton component in your plugin you will get TypeScript errors related to the change.
+
+**Recommended actions:**
+
+- Review use cases of IconButton in your plugin.
+- Add a meaningful tooltip which the component will also use as an aria-label.
+- Another option is to set an aria-label. In this case a tooltip will not be shown.
+
+**Please note:**
+The IconButton used to have a property called `ariaLabel` which got deprecated with this change. You can now use the regular property `aria-label` instead.
diff --git a/packages/grafana-ui/src/components/IconButton/IconButton.mdx b/packages/grafana-ui/src/components/IconButton/IconButton.mdx
@@ -1,17 +1,23 @@
 import { Meta, ArgTypes } from '@storybook/blocks';
 import { IconButton } from './IconButton';
+import { Icon } from '../Icon/Icon';
 import { Alert } from '../Alert/Alert';
 
 <Meta title="MDX|IconButton" component={IconButton} />
 
 # IconButton
 
-This component looks just like an icon but behaves like a button. It fulfils an action when you click it and has hover and focus states. You can choose which icon size you would like to use.
+This component looks just like an icon but behaves like a button. It fulfils an action when you click it and has a hover as well a focus states. You can choose which icon size you would like to use.
 
 `IconButton` is best used when you only want an icon instead of a button with text, for example when you want to place a solitary clickable icon next to text. An example where an `IconButton` is used in Grafana is the hamburger icon at the top left which opens the new navigation.
 When using `IconButton` right next to a text element consider wrapping both in a flex container and use `align-items: center;` to make them align properly.
 
-Always keep in mind to add text for a tooltip and an aria label.
+There are two options to use the IconButton:
+
+- with `Tooltip`: This is the preferred option since we don't want to rely on assumptions when it comes to the meaning an `Icon` has. Add a text for the `Tooltip`. It will be used for the `aria-label` as well.
+- without `Tooltip`: This is an option for use cases where the `Icon` is unambiguous e.g <Icon name="angle-down" /> for expanding a folder. Add a text for the `aria-label` and there will **not** be a `Tooltip`.
+
+The IconButton used to have a property called `ariaLabel` which got deprecated. You can now use the regular property `aria-label` instead.
 
 <Alert severity="warning" title={'Please note:'}>
   After reviewing this component we would like you to know that there are only 5 sizes available (sizes xs to xl). Sizes

diff --git a/packages/grafana-ui/src/components/IconButton/IconButton.story.tsx b/packages/grafana-ui/src/components/IconButton/IconButton.story.tsx
@@ -7,13 +7,16 @@ import { IconSize, IconName } from '../../types';
 import { withCenteredStory } from '../../utils/storybook/withCenteredStory';
 import { HorizontalGroup, VerticalGroup } from '../Layout/Layout';
 
-import { IconButton, IconButtonVariant, Props as IconButtonProps } from './IconButton';
+import { BasePropsWithTooltip, IconButton, IconButtonVariant, Props as IconButtonProps } from './IconButton';
 import mdx from './IconButton.mdx';
 
 interface ScenarioProps {
   background: 'canvas' | 'primary' | 'secondary';
 }
 
+const defaultExcludes = ['ariaLabel', 'aria-label'];
+const additionalExcludes = ['size', 'name', 'variant', 'iconType'];
+
 const meta: Meta<typeof IconButton> = {
   title: 'Buttons/IconButton',
   component: IconButton,
@@ -22,6 +25,7 @@ const meta: Meta<typeof IconButton> = {
     docs: {
       page: mdx,
     },
+    controls: { exclude: defaultExcludes },
   },
   args: {
     name: 'apps',
@@ -30,7 +34,8 @@ const meta: Meta<typeof IconButton> = {
     tooltip: 'sample tooltip message',
     tooltipPlacement: 'top',
     variant: 'secondary',
-    ariaLabel: 'sample aria-label content',
+    ariaLabel: 'this property is deprecated',
+    ['aria-label']: 'sample aria-label content',
   },
   argTypes: {
     tooltip: {
@@ -43,7 +48,7 @@ export const Basic: StoryFn<typeof IconButton> = (args: IconButtonProps) => {
   return <IconButton {...args} />;
 };
 
-export const ExamplesSizes = () => {
+export const ExamplesSizes = (args: BasePropsWithTooltip) => {
   const theme = useTheme2();
   const sizes: IconSize[] = ['xs', 'sm', 'md', 'lg', 'xl'];
   const icons: IconName[] = ['search', 'trash-alt', 'arrow-left', 'times'];
@@ -56,17 +61,22 @@ export const ExamplesSizes = () => {
   `;
 
   return (
-    <HorizontalGroup spacing="md">
+    <HorizontalGroup justify="center">
       {variants.map((variant) => {
         return (
-          <div key={variant}>
+          <div
+            key={variant}
+            className={css`
+              margin: auto ${theme.spacing(1)};
+            `}
+          >
             <p>{variant}</p>
             {icons.map((icon) => {
               return (
                 <div className={rowStyle} key={icon}>
                   {sizes.map((size) => (
                     <span key={icon + size}>
-                      <IconButton name={icon} size={size} variant={variant} tooltip="Tooltip example" />
+                      <IconButton name={icon} size={size} variant={variant} tooltip={args.tooltip} />
                     </span>
                   ))}
                 </div>
@@ -81,7 +91,7 @@ export const ExamplesSizes = () => {
           <div className={rowStyle} key={icon}>
             {sizes.map((size) => (
               <span key={icon + size}>
-                <IconButton name={icon} size={size} tooltip="Tooltip example" disabled />
+                <IconButton name={icon} size={size} tooltip={args.tooltip} disabled />
               </span>
             ))}
           </div>
@@ -91,7 +101,42 @@ export const ExamplesSizes = () => {
   );
 };
 
-export const ExamplesBackground = () => {
+ExamplesSizes.parameters = {
+  controls: {
+    exclude: [...defaultExcludes, ...additionalExcludes],
+  },
+};
+
+export const ExamplesBackground = (args: BasePropsWithTooltip) => {
+  const RenderBackgroundScenario = ({ background }: ScenarioProps) => {
+    const theme = useTheme2();
+    const variants: IconButtonVariant[] = ['primary', 'secondary', 'destructive'];
+
+    return (
+      <div
+        className={css`
+          padding: 30px;
+          background: ${theme.colors.background[background]};
+        `}
+      >
+        <VerticalGroup spacing="md">
+          <div>{background}</div>
+          <div
+            className={css`
+              display: flex;
+              gap: ${theme.spacing(2)};
+            `}
+          >
+            {variants.map((variant) => {
+              return <IconButton name="times" size="xl" variant={variant} key={variant} tooltip={args.tooltip} />;
+            })}
+            <IconButton name="times" size="xl" tooltip={args.tooltip} disabled />
+          </div>
+        </VerticalGroup>
+      </div>
+    );
+  };
+
   return (
     <div>
       <RenderBackgroundScenario background="canvas" />
@@ -101,33 +146,10 @@ export const ExamplesBackground = () => {
   );
 };
 
-const RenderBackgroundScenario = ({ background }: ScenarioProps) => {
-  const theme = useTheme2();
-  const variants: IconButtonVariant[] = ['primary', 'secondary', 'destructive'];
-
-  return (
-    <div
-      className={css`
-        padding: 30px;
-        background: ${theme.colors.background[background]};
-      `}
-    >
-      <VerticalGroup spacing="md">
-        <div>{background}</div>
-        <div
-          className={css`
-            display: flex;
-            gap: ${theme.spacing(2)};
-          `}
-        >
-          {variants.map((variant) => {
-            return <IconButton name="times" size="xl" variant={variant} key={variant} tooltip="Tooltip example" />;
-          })}
-          <IconButton name="times" size="xl" tooltip="Tooltip example" disabled />
-        </div>
-      </VerticalGroup>
-    </div>
-  );
+ExamplesBackground.parameters = {
+  controls: {
+    exclude: [...defaultExcludes, ...additionalExcludes],
+  },
 };
 
 export default meta;
diff --git a/packages/grafana-ui/src/components/IconButton/IconButton.tsx b/packages/grafana-ui/src/components/IconButton/IconButton.tsx
@@ -15,76 +15,92 @@ export type IconButtonVariant = 'primary' | 'secondary' | 'destructive';
 
 type LimitedIconSize = ComponentSize | 'xl';
 
-export interface Props extends React.ButtonHTMLAttributes<HTMLButtonElement> {
+interface BaseProps extends Omit<React.ButtonHTMLAttributes<HTMLButtonElement>, 'aria-label'> {
   /** Name of the icon **/
   name: IconName;
   /** Icon size - sizes xxl and xxxl are deprecated and when used being decreased to xl*/
   size?: IconSize;
   /** Type of the icon - mono or default */
   iconType?: IconType;
-  /** Tooltip content to display on hover */
-  tooltip?: PopoverContent;
-  /** Position of the tooltip */
-  tooltipPlacement?: TooltipPlacement;
   /** Variant to change the color of the Icon */
   variant?: IconButtonVariant;
-  /** Text available only for screen readers. Will use tooltip text as fallback. */
+}
+
+export interface BasePropsWithTooltip extends BaseProps {
+  /** Tooltip content to display on hover and as the aria-label */
+  tooltip: PopoverContent;
+  /** Position of the tooltip */
+  tooltipPlacement?: TooltipPlacement;
+}
+
+interface BasePropsWithAriaLabel extends BaseProps {
+  /** @deprecated use aria-label instead*/
   ariaLabel?: string;
+  /** Text available only for screen readers. No tooltip will be set in this case. */
+  ['aria-label']: string;
 }
 
-export const IconButton = React.forwardRef<HTMLButtonElement, Props>(
-  (
-    {
-      name,
-      size = 'md',
-      iconType,
-      tooltip,
-      tooltipPlacement,
-      ariaLabel,
-      className,
-      variant = 'secondary',
-      ...restProps
-    },
-    ref
-  ) => {
-    const theme = useTheme2();
-    let limitedIconSize: LimitedIconSize;
-
-    // very large icons (xl to xxxl) are unified to size xl
-    if (size === 'xxl' || size === 'xxxl') {
-      deprecationWarning('IconButton', 'size="xxl" and size="xxxl"', 'size="xl"');
-      limitedIconSize = 'xl';
-    } else {
-      limitedIconSize = size;
-    }
-
-    const styles = getStyles(theme, limitedIconSize, variant);
-    const tooltipString = typeof tooltip === 'string' ? tooltip : '';
-
-    // When using tooltip, ref is forwarded to Tooltip component instead for https://github.com/grafana/grafana/issues/65632
-    const button = (
+export type Props = BasePropsWithTooltip | BasePropsWithAriaLabel;
+
+export const IconButton = React.forwardRef<HTMLButtonElement, Props>((props, ref) => {
+  const { size = 'md', variant = 'secondary' } = props;
+
+  const theme = useTheme2();
+  let limitedIconSize: LimitedIconSize;
+
+  // very large icons (xl to xxxl) are unified to size xl
+  if (size === 'xxl' || size === 'xxxl') {
+    deprecationWarning('IconButton', 'size="xxl" and size="xxxl"', 'size="xl"');
+    limitedIconSize = 'xl';
+  } else {
+    limitedIconSize = size;
+  }
+
+  const styles = getStyles(theme, limitedIconSize, variant);
+
+  let ariaLabel: string | undefined;
+  let buttonRef: typeof ref | undefined;
+
+  if ('tooltip' in props) {
+    const { tooltip } = props;
+    ariaLabel = typeof tooltip === 'string' ? tooltip : undefined;
+  } else if ('ariaLabel' in props || 'aria-label' in props) {
+    const { ariaLabel: deprecatedAriaLabel, ['aria-label']: ariaLabelProp } = props;
+    ariaLabel = ariaLabelProp || deprecatedAriaLabel;
+    buttonRef = ref;
+  }
+
+  // When using tooltip, ref is forwarded to Tooltip component instead for https://github.com/grafana/grafana/issues/65632
+  if ('tooltip' in props) {
+    const { name, iconType, className, tooltip, tooltipPlacement, ...restProps } = props;
+    return (
+      <Tooltip ref={ref} content={tooltip} placement={tooltipPlacement}>
+        <button
+          {...restProps}
+          ref={buttonRef}
+          aria-label={ariaLabel}
+          className={cx(styles.button, className)}
+          type="button"
+        >
+          <Icon name={name} size={limitedIconSize} className={styles.icon} type={iconType} />
+        </button>
+      </Tooltip>
+    );
+  } else {
+    const { name, iconType, className, ...restProps } = props;
+    return (
       <button
-        ref={tooltip ? undefined : ref}
-        aria-label={ariaLabel || tooltipString}
         {...restProps}
+        ref={buttonRef}
+        aria-label={ariaLabel}
         className={cx(styles.button, className)}
         type="button"
       >
         <Icon name={name} size={limitedIconSize} className={styles.icon} type={iconType} />
       </button>
     );
-
-    if (tooltip) {
-      return (
-        <Tooltip ref={ref} content={tooltip} placement={tooltipPlacement}>
-          {button}
-        </Tooltip>
-      );
-    }
-
-    return button;
   }
-);
+});
 
 IconButton.displayName = 'IconButton';
 

diff --git a/packages/grafana-ui/src/components/Modal/Modal.tsx b/packages/grafana-ui/src/components/Modal/Modal.tsx
@@ -84,7 +84,7 @@ export function Modal(props: PropsWithChildren<Props>) {
               typeof title !== 'string' && title
             }
             <div className={styles.modalHeaderClose}>
-              <IconButton aria-label="Close dialog" name="times" size="xl" onClick={onDismiss} tooltip="Close" />
+              <IconButton name="times" size="xl" onClick={onDismiss} tooltip="Close" />
             </div>
           </div>
           <div className={cx(styles.modalContent, contentClassName)}>{children}</div>

diff --git a/packages/grafana-ui/src/components/Select/SelectBase.test.tsx b/packages/grafana-ui/src/components/Select/SelectBase.test.tsx
@@ -233,7 +233,7 @@ describe('SelectBase', () => {
 
       expect(screen.getByLabelText('My select')).toBeInTheDocument();
 
-      await userEvent.click(screen.getByLabelText('Remove Option 1'));
+      await userEvent.click(screen.getAllByLabelText('Remove')[0]);
       expect(onChangeHandler).toHaveBeenCalledWith([], {
         action: 'remove-value',
         name: undefined,

diff --git a/packages/grafana-ui/src/components/TagsInput/TagItem.tsx b/packages/grafana-ui/src/components/TagsInput/TagItem.tsx
@@ -28,8 +28,7 @@ export const TagItem = ({ name, disabled, onRemove }: Props) => {
         name="times"
         size="lg"
         disabled={disabled}
-        ariaLabel={`Remove "${name}" tag`}
-        tooltip="Remove tag"
+        tooltip={`Remove "${name}" tag`}
         onClick={() => onRemove(name)}
         className={styles.buttonStyles}
       />