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
This commit is contained in:
Laura Benz 2023-06-23 17:10:37 +02:00 committed by GitHub
parent 8dc9fcf88b
commit d64b6264ff
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
22 changed files with 185 additions and 117 deletions

View File

@ -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.

View File

@ -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

View File

@ -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;

View File

@ -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. */
ariaLabel?: 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;
export interface BasePropsWithTooltip extends BaseProps {
/** Tooltip content to display on hover and as the aria-label */
tooltip: PopoverContent;
/** Position of the tooltip */
tooltipPlacement?: TooltipPlacement;
}
// 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;
}
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;
}
const styles = getStyles(theme, limitedIconSize, variant);
const tooltipString = typeof tooltip === 'string' ? tooltip : '';
export type Props = BasePropsWithTooltip | BasePropsWithAriaLabel;
// When using tooltip, ref is forwarded to Tooltip component instead for https://github.com/grafana/grafana/issues/65632
const button = (
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';

View File

@ -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>

View File

@ -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,

View File

@ -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}
/>

View File

@ -25,15 +25,15 @@ describe('LayerDragDropList', () => {
it('showActions', () => {
renderScenario({ showActions: () => true });
expect(screen.getAllByLabelText('Duplicate button').length).toEqual(2);
expect(screen.getAllByLabelText('Remove button').length).toEqual(2);
expect(screen.getAllByLabelText('Duplicate').length).toEqual(2);
expect(screen.getAllByLabelText('Remove').length).toEqual(2);
});
it('showActions - no duplicate', () => {
renderScenario({ showActions: () => true, onDuplicate: undefined });
expect(screen.getAllByLabelText('Remove button').length).toEqual(2);
expect(screen.queryAllByLabelText('Duplicate button').length).toEqual(0);
expect(screen.getAllByLabelText('Remove').length).toEqual(2);
expect(screen.queryAllByLabelText('Duplicate').length).toEqual(0);
});
it('renders draggable icon', () => {

View File

@ -81,7 +81,6 @@ export const LayerDragDropList = <T extends LayerElement>({
<IconButton
name="copy"
tooltip="Duplicate"
ariaLabel="Duplicate button"
className={style.actionIcon}
onClick={() => onDuplicate(element)}
/>
@ -90,7 +89,6 @@ export const LayerDragDropList = <T extends LayerElement>({
<IconButton
name="trash-alt"
tooltip="Remove"
ariaLabel="Remove button"
className={cx(style.actionIcon, style.dragIcon)}
onClick={() => onDelete(element)}
/>

View File

@ -24,11 +24,11 @@ describe('QueryOperationRowHeader', () => {
describe('collapsable property', () => {
test('should show the button to collapse the query row by default', () => {
setup();
expect(screen.getByLabelText('toggle collapse and expand query row')).toBeInTheDocument();
expect(screen.getByLabelText('Collapse query row')).toBeInTheDocument();
});
test('should hide the button to collapse the query row when collapsable is set as false', () => {
setup({ collapsable: false });
expect(screen.queryByLabelText('toggle collapse and expand query row')).not.toBeInTheDocument();
expect(screen.queryByLabelText('Collapse query row')).not.toBeInTheDocument();
});
});
});

View File

@ -40,7 +40,6 @@ export const QueryOperationRowHeader = ({
{collapsable && (
<IconButton
name={isContentVisible ? 'angle-down' : 'angle-right'}
aria-label="toggle collapse and expand query row"
tooltip={isContentVisible ? 'Collapse query row' : 'Expand query row'}
className={styles.collapseIcon}
onClick={onRowToggle}

View File

@ -72,7 +72,7 @@ describe('browse-dashboards DeleteModal', () => {
it('calls onDismiss when clicking the X', async () => {
render(<DeleteModal {...defaultProps} />);
await userEvent.click(await screen.findByRole('button', { name: 'Close dialog' }));
await userEvent.click(await screen.findByRole('button', { name: 'Close' }));
expect(mockOnDismiss).toHaveBeenCalled();
});
});

View File

@ -113,7 +113,7 @@ describe('browse-dashboards MoveModal', () => {
it('calls onDismiss when clicking the X', async () => {
render(<MoveModal {...props} />);
await userEvent.click(await screen.findByRole('button', { name: 'Close dialog' }));
await userEvent.click(await screen.findByRole('button', { name: 'Close' }));
expect(mockOnDismiss).toHaveBeenCalled();
});
});

View File

@ -74,7 +74,7 @@ export function NameCell({ row: { original: data }, onFolderClick }: NameCellPro
onFolderClick(item.uid, !isOpen);
}}
name={isOpen ? 'angle-down' : 'angle-right'}
ariaLabel={isOpen ? 'Collapse folder' : 'Expand folder'}
tooltip={isOpen ? 'Collapse folder' : 'Expand folder'}
/>
)}
</>

View File

@ -318,7 +318,7 @@ describe('LibraryPanelsSearch', () => {
expect(card()).toBeInTheDocument();
expect(within(card()).getByText(/library panel name/i)).toBeInTheDocument();
expect(within(card()).getByText(/library panel description/i)).toBeInTheDocument();
expect(within(card()).getByLabelText(/delete button on panel type card/i)).toBeInTheDocument();
expect(within(card()).getByLabelText(/Delete/i)).toBeInTheDocument();
});
});
@ -354,9 +354,9 @@ describe('LibraryPanelsSearch', () => {
}
);
await userEvent.click(screen.getByLabelText(/delete button on panel type card/i));
await userEvent.click(screen.getByLabelText('Delete'));
await waitFor(() => expect(screen.getByText('Do you want to delete this panel?')).toBeInTheDocument());
await userEvent.click(screen.getByRole('button', { name: 'Delete' }));
await userEvent.click(screen.getAllByRole('button', { name: 'Delete' })[1]);
await waitFor(() => {
expect(getLibraryPanelsSpy).toHaveBeenCalledWith({

View File

@ -96,7 +96,7 @@ describe('QueryEditorRows', () => {
const queryEditorRows = await screen.findAllByTestId('query-editor-row');
for (const childQuery of queryEditorRows) {
const toggleExpandButton = queryByLabelText(childQuery, 'toggle collapse and expand query row') as HTMLElement;
const toggleExpandButton = queryByLabelText(childQuery, 'Collapse query row') as HTMLElement;
expect(toggleExpandButton).toBeInTheDocument();
expect(toggleExpandButton.getAttribute('aria-expanded')).toBe('true');

View File

@ -80,7 +80,7 @@ describe('QueryGroup', () => {
await userEvent.click(addExpressionButton);
const lastQueryEditorRow = (await screen.findAllByTestId('query-editor-row')).at(-1);
const lastEditorToggleRow = (await screen.findAllByLabelText('toggle collapse and expand query row')).at(-1);
const lastEditorToggleRow = (await screen.findAllByLabelText('Collapse query row')).at(-1);
expect(lastEditorToggleRow?.getAttribute('aria-expanded')).toBe('true');
expect(lastQueryEditorRow?.firstElementChild?.children.length).toBe(2);
@ -97,7 +97,7 @@ describe('QueryGroup', () => {
await userEvent.click(addQueryButton);
const lastQueryEditorRow = (await screen.findAllByTestId('query-editor-row')).at(-1);
const lastEditorToggleRow = (await screen.findAllByLabelText('toggle collapse and expand query row')).at(-1);
const lastEditorToggleRow = (await screen.findAllByLabelText('Collapse query row')).at(-1);
expect(lastEditorToggleRow?.getAttribute('aria-expanded')).toBe('true');
expect(lastQueryEditorRow?.firstElementChild?.children.length).toBe(2);

View File

@ -132,8 +132,7 @@ const ServiceAccountListItem = memo(
name="trash-alt"
size="md"
onClick={() => onRemoveButtonClick(serviceAccount)}
aria-label={`Delete service account ${serviceAccount.name}`}
tooltip="Delete account"
tooltip={`Delete service account ${serviceAccount.name}`}
/>
)}
</HorizontalGroup>

View File

@ -201,8 +201,8 @@ describe(`Azure Monitor QueryEditor`, () => {
/>
);
await screen.findByText('testlabel');
const labelClear = await screen.findByLabelText('Remove testlabel');
await user.click(labelClear);
const labelClear = await screen.findAllByLabelText('Remove');
await user.click(labelClear[0]);
mockQuery = setDimensionFilterValue(mockQuery, 0, 'filters', []);
expect(onQueryChange).toHaveBeenCalledWith({
...mockQuery,

View File

@ -270,7 +270,7 @@ const Filter = (
onCloseMenu={() => onFieldChange('filters', item, selected, onChange)}
hideSelectedOptions={false}
/>
<AccessoryButton aria-label="Remove" icon="times" variant="secondary" onClick={onDelete} type="button" />
<AccessoryButton aria-label="Remove filter" icon="times" variant="secondary" onClick={onDelete} type="button" />
</HorizontalGroup>
);
};

View File

@ -281,7 +281,7 @@ describe(`Traces Filters`, () => {
rerender
);
const removeButtons = screen.getAllByLabelText('Remove');
const removeButtons = screen.getAllByLabelText('Remove filter');
mockQuery = {
...mockQuery,
@ -388,9 +388,9 @@ describe(`Traces Filters`, () => {
],
},
};
const removeLabel = screen.getByLabelText(`Remove test-app-id-2`);
const removeLabel = screen.getAllByLabelText(`Remove`);
await act(async () => {
await userEvent.click(removeLabel);
await userEvent.click(removeLabel[1]);
});
rerender(

View File

@ -112,8 +112,8 @@ describe('SearchField', () => {
expect(updateFilter).toHaveBeenCalledWith({ ...filter, value: ['driver', 'customer'] });
// Remove the first value
const firstValRemove = await screen.findByLabelText('Remove driver');
await user.click(firstValRemove);
const firstValRemove = await screen.findAllByLabelText('Remove');
await user.click(firstValRemove[0]);
expect(updateFilter).toHaveBeenCalledWith({ ...filter, value: ['customer'] });
}
});