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.

Sign up for GitHub

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

Update SvgIcon for the latest axe-core #540

Merged
merged 4 commits into from
Aug 2, 2021
Merged

Update SvgIcon for the latest axe-core #540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
187b06f
chore: upgrade axe-core from 4.1.2 to 4.3.2
ahuth Jul 28, 2021
712bdab
fix: use aria-hidden to hide icons from screen readers
ahuth Jul 28, 2021
f0222dc
feat: use "informative" and "decorative" as the type of icons
ahuth Jul 28, 2021
aadd789
chore: change <SvgIcon> "kind" prop to "purpose"
ahuth Aug 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/components/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/components/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@
"@types/jest": "^26.0.24",
"@types/react": "^17.0.2",
"autoprefixer": "^10.2.4",
"axe-core": "^4.1.2",
"axe-core": "^4.3.2",
"babel-jest": "^26.6.3",
"babel-loader": "^8.2.2",
"core-js": "^3.9.1",
Expand Down
20 changes: 12 additions & 8 deletions packages/components/src/Button/__snapshots__/button.spec.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,14 @@ exports[`<Button /> linkWithIcon story renders snapshot 1`] = `
​
Link with icon
<svg
role="presentation"
style="height: 24px;"
viewBox="0 0 20 20"
aria-hidden="true"
class="svgIcon"
fill="currentColor"
viewBox="0 0 24 24"
xmlns="http://www.w3.org/2000/svg"
>
<path
d="M9.719,17.073l-6.562-6.51c-0.27-0.268-0.504-0.567-0.696-0.888C1.385,7.89,1.67,5.613,3.155,4.14c0.864-0.856,2.012-1.329,3.233-1.329c1.924,0,3.115,1.12,3.612,1.752c0.499-0.634,1.689-1.752,3.612-1.752c1.221,0,2.369,0.472,3.233,1.329c1.484,1.473,1.771,3.75,0.693,5.537c-0.19,0.32-0.425,0.618-0.695,0.887l-6.562,6.51C10.125,17.229,9.875,17.229,9.719,17.073 M6.388,3.61C5.379,3.61,4.431,4,3.717,4.707C2.495,5.92,2.259,7.794,3.145,9.265c0.158,0.265,0.351,0.51,0.574,0.731L10,16.228l6.281-6.232c0.224-0.221,0.416-0.466,0.573-0.729c0.887-1.472,0.651-3.346-0.571-4.56C15.57,4,14.621,3.61,13.612,3.61c-1.43,0-2.639,0.786-3.268,1.863c-0.154,0.264-0.536,0.264-0.69,0C9.029,4.397,7.82,3.61,6.388,3.61"
d="M12 2C6.48 2 2 6.48 2 12s4.48 10 10 10 10-4.48 10-10S17.52 2 12 2zm-2 15l-5-5 1.41-1.41L10 14.17l7.59-7.59L19 8l-9 9z"
/>
</svg>
</button>
Expand All @@ -89,12 +91,14 @@ exports[`<Button /> outlineWithIcon story renders snapshot 1`] = `
​
Outline with icon
<svg
role="presentation"
style="height: 24px;"
viewBox="0 0 20 20"
aria-hidden="true"
class="svgIcon"
fill="currentColor"
viewBox="0 0 24 24"
xmlns="http://www.w3.org/2000/svg"
>
<path
d="M9.719,17.073l-6.562-6.51c-0.27-0.268-0.504-0.567-0.696-0.888C1.385,7.89,1.67,5.613,3.155,4.14c0.864-0.856,2.012-1.329,3.233-1.329c1.924,0,3.115,1.12,3.612,1.752c0.499-0.634,1.689-1.752,3.612-1.752c1.221,0,2.369,0.472,3.233,1.329c1.484,1.473,1.771,3.75,0.693,5.537c-0.19,0.32-0.425,0.618-0.695,0.887l-6.562,6.51C10.125,17.229,9.875,17.229,9.719,17.073 M6.388,3.61C5.379,3.61,4.431,4,3.717,4.707C2.495,5.92,2.259,7.794,3.145,9.265c0.158,0.265,0.351,0.51,0.574,0.731L10,16.228l6.281-6.232c0.224-0.221,0.416-0.466,0.573-0.729c0.887-1.472,0.651-3.346-0.571-4.56C15.57,4,14.621,3.61,13.612,3.61c-1.43,0-2.639,0.786-3.268,1.863c-0.154,0.264-0.536,0.264-0.69,0C9.029,4.397,7.82,3.61,6.388,3.61"
d="M12 2C6.48 2 2 6.48 2 12s4.48 10 10 10 10-4.48 10-10S17.52 2 12 2zm-2 15l-5-5 1.41-1.41L10 14.17l7.59-7.59L19 8l-9 9z"
/>
</svg>
</button>
Expand Down
19 changes: 11 additions & 8 deletions packages/components/src/Button/button.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import Button from "./button";
import { CheckCircle } from "../Icons";
import React from "react";
import { Story } from "@storybook/react/types-6-0";
import Text from "../Text";
Expand All @@ -19,12 +20,6 @@ type Args = React.ComponentProps<typeof Button>;

const Template: Story<Args> = (args) => <Button {...args} />;

const heartIcon = (
<svg style={{ height: "24px" }} viewBox="0 0 20 20" role="presentation">
<path d="M9.719,17.073l-6.562-6.51c-0.27-0.268-0.504-0.567-0.696-0.888C1.385,7.89,1.67,5.613,3.155,4.14c0.864-0.856,2.012-1.329,3.233-1.329c1.924,0,3.115,1.12,3.612,1.752c0.499-0.634,1.689-1.752,3.612-1.752c1.221,0,2.369,0.472,3.233,1.329c1.484,1.473,1.771,3.75,0.693,5.537c-0.19,0.32-0.425,0.618-0.695,0.887l-6.562,6.51C10.125,17.229,9.875,17.229,9.719,17.073 M6.388,3.61C5.379,3.61,4.431,4,3.717,4.707C2.495,5.92,2.259,7.794,3.145,9.265c0.158,0.265,0.351,0.51,0.574,0.731L10,16.228l6.281-6.232c0.224-0.221,0.416-0.466,0.573-0.729c0.887-1.472,0.651-3.346-0.571-4.56C15.57,4,14.621,3.61,13.612,3.61c-1.43,0-2.639,0.786-3.268,1.863c-0.154,0.264-0.536,0.264-0.69,0C9.029,4.397,7.82,3.61,6.388,3.61"></path>
</svg>
);

export const flat = Template.bind(null);
flat.args = {
children: "Flat Button",
Expand Down Expand Up @@ -79,14 +74,22 @@ withDataTestId.args = {

export const linkWithIcon = Template.bind(null);
linkWithIcon.args = {
children: <>Link with icon {heartIcon}</>,
children: (
<>
Link with icon <CheckCircle purpose="decorative" />
</>
),
color: "success",
variant: "link",
};

export const outlineWithIcon = Template.bind(null);
outlineWithIcon.args = {
children: <>Outline with icon {heartIcon}</>,
children: (
<>
Outline with icon <CheckCircle purpose="decorative" />
</>
),
color: "warning",
variant: "outline",
};
Expand Down
14 changes: 7 additions & 7 deletions packages/components/src/SvgIcon/SvgIcon.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ export default {
options: Object.keys(allIcons),
},
},
role: {
purpose: {
control: {
type: "radio",
options: ["img", "presentation"],
options: ["informative", "decorative"],
},
},
size: {
Expand Down Expand Up @@ -49,7 +49,7 @@ const Template: Story<Args> = ({ icon, color, ...rest }) => {

const defaultArgs = {
icon: Object.keys(allIcons)[0] as keyof typeof allIcons,
role: "presentation" as const,
role: "decorative" as const,
};

export const Small = Template.bind(null);
Expand Down Expand Up @@ -100,11 +100,11 @@ export const InText = ({ icon, ...rest }: Args) => {
return (
<Text size="h1">
The svg icon defaults to the surrounding text size (
<Icon {...rest} role="img" title="icon with 1em line height" />, 1em), but
often looks better with the line height (
<Icon {...rest} purpose="informative" title="icon with 1em line height" />
, 1em), but often looks better with the line height (
<Icon
{...rest}
role="img"
purpose="informative"
title="icon with 2em line height"
size="2rem"
/>
Expand All @@ -130,7 +130,7 @@ export const AllIcons = () => {
<ul className={styles.allIcons}>
{Object.entries(allIcons).map(([name, Icon]: [string, any]) => (
<li className={styles.iconCell} key={name}>
<Icon block role="presentation" size="2rem" />
<Icon block type="decorative" size="2rem" />
<Text as="span" size="sm">
{name}
</Text>
Expand Down
39 changes: 27 additions & 12 deletions packages/components/src/SvgIcon/SvgIcon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,20 +35,33 @@ interface IconPropsBase {
viewBox?: string;
}

interface FunctionalIconProps extends IconPropsBase {
role: "img";
interface InformativeIconProps extends IconPropsBase {
/**
* The role of the icon.
*
* Use "informative" when the icon **_does_** provide additional meaning to other text on the
* page. You'll be required to pass in a title to label the icon.
*/
purpose: "informative";
title: string;
}

interface PresentationalIconProps extends IconPropsBase {
role: "presentation";
interface DecorativeIconProps extends IconPropsBase {
/**
* The role of the icon.
*
* Use "decorative" when the icon **_does not_** provide any additional context or meaning to
* associated text. Basically the icon is for show and people don't need it to understand what's
* on the page.
*/
purpose: "decorative";
}

export type SvgIconProps = PresentationalIconProps | FunctionalIconProps;
export type SvgIconProps = DecorativeIconProps | InformativeIconProps;

export type IconProps =
| Omit<PresentationalIconProps, "children">
| Omit<FunctionalIconProps, "children">;
| Omit<DecorativeIconProps, "children">
| Omit<InformativeIconProps, "children">;

interface SvgStyle extends React.CSSProperties {
"--svg-icon-size"?: string;
Expand All @@ -65,7 +78,6 @@ function SvgIcon(props: SvgIconProps) {
children,
className,
color = "currentColor",
role,
size,
viewBox = "0 0 24 24",
} = props;
Expand All @@ -78,7 +90,6 @@ function SvgIcon(props: SvgIconProps) {
className: clsx(className, styles.svgIcon, block && styles.displayBlock),
fill: color,
height: size,
role,
/**
* height/width html properties are overriden by the defaults applied
* to svg css class
Expand All @@ -89,15 +100,19 @@ function SvgIcon(props: SvgIconProps) {
xmlns: "http://www.w3.org/2000/svg",
};

if (props.role === "img") {
if (props.purpose === "informative") {
return (
<svg {...svgCommonProps}>
<svg {...svgCommonProps} role="img">
<title>{props.title}</title>
Comment on lines +105 to 106
Copy link
Contributor

Choose a reason for hiding this comment

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

I kinda wonder if we should use an aria-label and drop the role + title for informative icons for consistency. It feels a little weird to me that one version of the svgs has a role and the other doesn't. 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

One benefit of <title> (which our auditors request sometimes) is it'll appear when hovering on the icon, in case it's not clear what an icon means. I do generally like aria-label better, but that could be a reason to keep this

{children}
</svg>
);
} else {
return <svg {...svgCommonProps}>{children}</svg>;
return (
<svg {...svgCommonProps} aria-hidden>
{children}
</svg>
);
}
}

Expand Down
12 changes: 6 additions & 6 deletions packages/components/src/SvgIcon/__snapshots__/SvgIcon.spec.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@

exports[`<SvgIcon /> CustomColor story renders snapshot 1`] = `
<svg
aria-hidden="true"
class="svgIcon"
fill="#928CFF"
role="presentation"
viewBox="0 0 24 24"
xmlns="http://www.w3.org/2000/svg"
>
Expand All @@ -16,9 +16,9 @@ exports[`<SvgIcon /> CustomColor story renders snapshot 1`] = `

exports[`<SvgIcon /> CustomViewBox story renders snapshot 1`] = `
<svg
aria-hidden="true"
class="svgIcon"
fill="currentColor"
role="presentation"
viewBox="4 4 16 16"
xmlns="http://www.w3.org/2000/svg"
>
Expand All @@ -30,10 +30,10 @@ exports[`<SvgIcon /> CustomViewBox story renders snapshot 1`] = `

exports[`<SvgIcon /> FullScreen story renders snapshot 1`] = `
<svg
aria-hidden="true"
class="svgIcon"
fill="currentColor"
height="min(100%, 100vh)"
role="presentation"
style="--svg-icon-size: min(100%, 100vh);"
viewBox="0 0 24 24"
width="min(100%, 100vh)"
Expand Down Expand Up @@ -97,10 +97,10 @@ exports[`<SvgIcon /> InText story renders snapshot 1`] = `

exports[`<SvgIcon /> Large story renders snapshot 1`] = `
<svg
aria-hidden="true"
class="svgIcon"
fill="currentColor"
height="4em"
role="presentation"
style="--svg-icon-size: 4em;"
viewBox="0 0 24 24"
width="4em"
Expand All @@ -114,10 +114,10 @@ exports[`<SvgIcon /> Large story renders snapshot 1`] = `

exports[`<SvgIcon /> Medium story renders snapshot 1`] = `
<svg
aria-hidden="true"
class="svgIcon"
fill="currentColor"
height="2em"
role="presentation"
style="--svg-icon-size: 2em;"
viewBox="0 0 24 24"
width="2em"
Expand All @@ -131,10 +131,10 @@ exports[`<SvgIcon /> Medium story renders snapshot 1`] = `

exports[`<SvgIcon /> Small story renders snapshot 1`] = `
<svg
aria-hidden="true"
class="svgIcon"
fill="currentColor"
height="1em"
role="presentation"
style="--svg-icon-size: 1em;"
viewBox="0 0 24 24"
width="1em"
Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/Tag/Tag.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,12 @@ OutlineVariants.args = {
export const WithIcon = Template.bind(null);
WithIcon.args = {
...defaultArgs,
icon: <WarningIcon key="icon" role="img" title="warning" />,
icon: <WarningIcon key="icon" purpose="informative" title="warning" />,
};

export const WithLongTextAndIcon = Template.bind(null);
WithLongTextAndIcon.args = {
...defaultArgs,
children: "This tag has a really long text message",
icon: <WarningIcon key="icon" role="img" title="warning" />,
icon: <WarningIcon key="icon" purpose="informative" title="warning" />,
};
2 changes: 1 addition & 1 deletion packages/components/src/common/CloseButton/CloseButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ const CloseButton = ({ className, color, onClose, size }: CloseButtonProps) => (
onClick={onClose}
variant="link"
>
<CloseIcon role="img" size={size} title="close" />
<CloseIcon purpose="informative" size={size} title="close" />
</Button>
);

Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/common/Notifications/NotificationIcon/NotificationIcon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ const NotificationIcon = ({ variant }: Props) => {

return (
<div className={iconAssets.style}>
<iconAssets.icon role="img" title={iconAssets.title} />
<iconAssets.icon purpose="informative" title={iconAssets.title} />
</div>
);
};
Expand Down