Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 23 commits into from Jun 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
65b3372
refactor: modify interfaces to make tooltip or aria-label required
L-M-K-B Jun 7, 2023
d6eb2e7
refactor: change functionality around aria-label and tooltip
L-M-K-B Jun 12, 2023
f18ccdf
refactor: change and add information in storybook documentation
L-M-K-B Jun 12, 2023
84edb8f
refactor: remove default from tooltip
L-M-K-B Jun 12, 2023
8f5bfb4
refactor: IconButton to make tooltip or aria-label required
L-M-K-B Jun 13, 2023
7f3e3e4
refactor: Fix tests
L-M-K-B Jun 13, 2023
8b8ecef
refactor: Fix tests
L-M-K-B Jun 14, 2023
bc59a5e
refactor: Fix tests
L-M-K-B Jun 15, 2023
1d4ebca
refactor: Fix tests
L-M-K-B Jun 16, 2023
86c754c
feat: add migration guide for breaking change
L-M-K-B Jun 16, 2023
a30c240
feat: add latest requirements to storybook docs
L-M-K-B Jun 16, 2023
b5c8394
refactor: separate iconbutton story with and without tooltip
L-M-K-B Jun 21, 2023
e5b24cf
refactor: remove exported baseArgs
L-M-K-B Jun 21, 2023
bc36f41
refactor: clean up and restructure original story
L-M-K-B Jun 22, 2023
f2ba95a
refactor: adjust styling
L-M-K-B Jun 22, 2023
a96fe97
refactor: enable control for tooltip
L-M-K-B Jun 22, 2023
05b55c5
refactor: clean up
L-M-K-B Jun 22, 2023
130ba4c
refactor: enable control for aria-label
L-M-K-B Jun 22, 2023
a2a0a40
refactor: fix theme getting the wrong theme
L-M-K-B Jun 22, 2023
e590ed3
refactor: fix tests
L-M-K-B Jun 22, 2023
c7d48e3
refactor: adjust story
L-M-K-B Jun 23, 2023
a9da5c6
refactor: remove confusing story
L-M-K-B Jun 23, 2023
dc97eed
refactor: adjust controls for stories
L-M-K-B Jun 23, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -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.
10 changes: 8 additions & 2 deletions 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
Expand Down
Expand Up @@ -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,
Expand All @@ -22,6 +25,7 @@ const meta: Meta<typeof IconButton> = {
docs: {
page: mdx,
},
controls: { exclude: defaultExcludes },
},
args: {
name: 'apps',
Expand All @@ -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',
L-M-K-B marked this conversation as resolved.
Show resolved Hide resolved
['aria-label']: 'sample aria-label content',
},
argTypes: {
tooltip: {
Expand All @@ -43,7 +48,7 @@ export const Basic: StoryFn<typeof IconButton> = (args: IconButtonProps) => {
return <IconButton {...args} />;
};

export const ExamplesSizes = () => {
export const ExamplesSizes = (args: BasePropsWithTooltip) => {
L-M-K-B marked this conversation as resolved.
Show resolved Hide resolved
const theme = useTheme2();
const sizes: IconSize[] = ['xs', 'sm', 'md', 'lg', 'xl'];
const icons: IconName[] = ['search', 'trash-alt', 'arrow-left', 'times'];
Expand All @@ -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>
Expand All @@ -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>
Expand All @@ -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" />
Expand All @@ -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;
116 changes: 66 additions & 50 deletions packages/grafana-ui/src/components/IconButton/IconButton.tsx
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to discriminate further than this because you need to check if the Tooltip passed is a string or a component. If it is a string it can be used as the aria-label, if it is a component the aria-label will have to be explicitly set still.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ckbedwell I changed the component quite a lot and after discussing what if the tooltip was not a string we decided to keep it as undefined for now. This use case doesn't occur too often so it should be fine.

We also discussed within the Frontend Platform team whether it makes sense to always require a tooltip and we identified some use cases in Grafana where a tooltip is not useful. For instance when IconButtons are used to expand context. They show a chevron and we think the icon is unambiguous in this case.

/** 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;
L-M-K-B marked this conversation as resolved.
Show resolved Hide resolved

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"
>
L-M-K-B marked this conversation as resolved.
Show resolved Hide resolved
<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';

Expand Down
2 changes: 1 addition & 1 deletion packages/grafana-ui/src/components/Modal/Modal.tsx
Expand Up @@ -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>
Expand Down
Expand Up @@ -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,
Expand Down
3 changes: 1 addition & 2 deletions packages/grafana-ui/src/components/TagsInput/TagItem.tsx
Expand Up @@ -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}
/>
Expand Down