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

Revert "Accessibility engineering review for Link component." #3459

Merged
merged 1 commit into from
Jun 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 0 additions & 6 deletions .changeset/curly-otters-repeat.md

This file was deleted.

4 changes: 4 additions & 0 deletions docs/content/Link.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ import data from '../../src/Link/Link.docs.json'

The Link component styles anchor tags with default hyperlink color cues and hover text decoration. `Link` is used for destinations, or moving from one page to another.

In special cases where you'd like a `<button>` styled like a `Link`, use `<Link as='button'>`. Make sure to provide a click handler with `onClick`.

**Important:** When using the `as` prop, be sure to always render an accessible element type, like `a`, `button`, `input`, or `summary`.

## Examples

```jsx live
Expand Down
14 changes: 12 additions & 2 deletions generated/components.json
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,11 @@
"name": "ref",
"type": "React.RefObject<HTMLAnchorElement>"
},
{
"name": "as",
"type": "React.ElementType",
"defaultValue": "\"a\""
},
{
"name": "sx",
"type": "SystemStyleObject"
Expand Down Expand Up @@ -895,7 +900,7 @@
},
{
"name": "variant",
"type": "| 'default'\n| 'primary'\n| 'danger'\n| 'outline'\n| 'invisible'\n| 'link'",
"type": "| 'default'\n| 'primary'\n| 'danger'\n| 'outline'\n| 'invisible'",
"defaultValue": "'default'",
"description": "Change the visual style of the button."
},
Expand Down Expand Up @@ -2057,7 +2062,7 @@
"name": "href",
"type": "string",
"defaultValue": "",
"description": "URL to be used for the Link. (The `href` is passed to the underlying `<a>` element)."
"description": "URL to be used for the Link. (The `href` is passed to the underlying `<a>` element. If `as` is specified, the link behavior may need different props)."
},
{
"name": "muted",
Expand All @@ -2081,6 +2086,11 @@
"name": "ref",
"type": "React.RefObject<HTMLAnchorElement>"
},
{
"name": "as",
"type": "React.ElementType",
"defaultValue": "\"a\""
},
{
"name": "sx",
"type": "SystemStyleObject"
Expand Down
5 changes: 5 additions & 0 deletions src/ActionList/ActionList.docs.json
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,11 @@
"name": "ref",
"type": "React.RefObject<HTMLAnchorElement>"
},
{
"name": "as",
"type": "React.ElementType",
"defaultValue": "\"a\""
},
{
"name": "sx",
"type": "SystemStyleObject"
Expand Down
14 changes: 9 additions & 5 deletions src/ActionList/LinkItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {ActionListItemProps} from './shared'
type LinkProps = {
download?: string
href?: string
to?: string
hrefLang?: string
media?: string
ping?: string
Expand All @@ -22,7 +21,7 @@ type LinkProps = {
// LinkItem does not support selected, variants, etc.
export type ActionListLinkItemProps = Pick<ActionListItemProps, 'active' | 'children' | 'sx'> & LinkProps

export const LinkItem = React.forwardRef(({sx = {}, active, ...props}, forwardedRef) => {
export const LinkItem = React.forwardRef(({sx = {}, active, as: Component, ...props}, forwardedRef) => {
const styles = {
// occupy full size of Item
paddingX: 2,
Expand All @@ -36,8 +35,6 @@ export const LinkItem = React.forwardRef(({sx = {}, active, ...props}, forwarded
'&:hover': {color: 'inherit', textDecoration: 'none'},
}

if (props.href === undefined && props.to !== undefined) props.href = props.to

return (
<Item
active={active}
Expand All @@ -48,7 +45,14 @@ export const LinkItem = React.forwardRef(({sx = {}, active, ...props}, forwarded
props.onClick && props.onClick(event as React.MouseEvent<HTMLAnchorElement>)
}
return (
<Link sx={merge(styles, sx as SxProp)} {...rest} {...props} onClick={clickHandler} ref={forwardedRef}>
<Link
as={Component}
sx={merge(styles, sx as SxProp)}
{...rest}
{...props}
onClick={clickHandler}
ref={forwardedRef}
>
{children}
</Link>
)
Expand Down
4 changes: 2 additions & 2 deletions src/Button/Button.docs.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
},
{
"name": "variant",
"type": "| 'default'\n| 'primary'\n| 'danger'\n| 'outline'\n| 'invisible'\n| 'link'",
"type": "| 'default'\n| 'primary'\n| 'danger'\n| 'outline'\n| 'invisible'",
"defaultValue": "'default'",
"description": "Change the visual style of the button."
},
Expand Down Expand Up @@ -67,4 +67,4 @@
]
}
]
}
}
2 changes: 1 addition & 1 deletion src/Button/Button.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export default {
control: {
type: 'radio',
},
options: ['default', 'primary', 'danger', 'invisible', 'outline', 'link'],
options: ['default', 'primary', 'danger', 'invisible', 'outline'],
},
alignContent: {
control: {
Expand Down
19 changes: 0 additions & 19 deletions src/Button/styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -186,25 +186,6 @@ export const getVariantStyles = (variant: VariantType = 'default', theme?: Theme
borderColor: 'btn.outline.selectedBorder',
},
},
link: {
color: 'accent.fg',
backgroundColor: 'transparent',
display: 'inline-flex',
border: 'none',
height: 'unset',
padding: 0,
'&:hover:not(:disabled)': {
textDecoration: 'underline',
},
'&:focus-visible, &:focus': {
outlineOffset: '2px',
},
'&:disabled, &[aria-disabled=true]': {
color: 'primer.fg.disabled',
backgroundColor: 'transparent',
borderColor: 'transparent',
},
},
}
return style[variant]
}
Expand Down
2 changes: 1 addition & 1 deletion src/Button/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export const StyledButton = styled.button<SxProp>`
${sx};
`

export type VariantType = 'default' | 'primary' | 'invisible' | 'danger' | 'outline' | 'link'
export type VariantType = 'default' | 'primary' | 'invisible' | 'danger' | 'outline'

export type Size = 'small' | 'medium' | 'large'

Expand Down
9 changes: 7 additions & 2 deletions src/Link/Link.docs.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"name": "href",
"type": "string",
"defaultValue": "",
"description": "URL to be used for the Link. (The `href` is passed to the underlying `<a>` element)."
"description": "URL to be used for the Link. (The `href` is passed to the underlying `<a>` element. If `as` is specified, the link behavior may need different props)."
},
{
"name": "muted",
Expand All @@ -33,10 +33,15 @@
"name": "ref",
"type": "React.RefObject<HTMLAnchorElement>"
},
{
"name": "as",
"type": "React.ElementType",
"defaultValue": "\"a\""
},
{
"name": "sx",
"type": "SystemStyleObject"
}
],
"subcomponents": []
}
}
20 changes: 1 addition & 19 deletions src/Link/Link.features.stories.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import Link from '../Link'
import Box from '../Box'
import {Meta, StoryFn} from '@storybook/react'
import {Meta} from '@storybook/react'
import React from 'react'
import {ComponentProps} from '../utils/types'

Expand All @@ -20,20 +19,3 @@ export const Underline = () => (
Link
</Link>
)

export const WithinText: StoryFn<typeof Link> = args => (
<Box as="p">
This{' '}
<Link href="#" {...args}>
link
</Link>{' '}
is inside of other text.
</Box>
)

WithinText.args = {
muted: false,
underline: false,
}

WithinText.argTypes = {}
46 changes: 37 additions & 9 deletions src/Link/Link.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {forwardRef} from 'react'
import React, {forwardRef, useEffect} from 'react'
import styled from 'styled-components'
import {system} from 'styled-system'
import {get} from '../constants'
Expand All @@ -23,27 +23,55 @@ const hoverColor = system({
const StyledLink = styled.a<StyledLinkProps>`
color: ${props => (props.muted ? get('colors.fg.muted')(props) : get('colors.accent.fg')(props))};
text-decoration: ${props => (props.underline ? 'underline' : 'none')};
&:hover,
&:focus {
&:hover {
text-decoration: ${props => (props.muted ? 'none' : 'underline')};
${props => (props.hoverColor ? hoverColor : props.muted ? `color: ${get('colors.accent.fg')(props)}` : '')};
}
&:is(button) {
display: inline-block;
padding: 0;
font-size: inherit;
white-space: nowrap;
cursor: pointer;
user-select: none;
background-color: transparent;
border: 0;
appearance: none;
}
${sx};
`

const Link = forwardRef(({as, ...props}, forwardedRef) => {
const Link = forwardRef(({as: Component = 'a', ...props}, forwardedRef) => {
const innerRef = React.useRef<HTMLAnchorElement>(null)
useRefObjectAsForwardedRef(forwardedRef, innerRef)

if (as !== undefined) {
// eslint-disable-next-line no-console
console.warn(
'Links no longer accept an as prop. If you need to style another tag as a link, you should use a different component and apply appropriate styling.',
)
if (__DEV__) {
/**
* The Linter yells because it thinks this conditionally calls an effect,
* but since this is a compile-time flag and not a runtime conditional
* this is safe, and ensures the entire effect is kept out of prod builds
* shaving precious bytes from the output, and avoiding mounting a noop effect
*/
// eslint-disable-next-line react-hooks/rules-of-hooks
useEffect(() => {
if (
innerRef.current &&
!(innerRef.current instanceof HTMLButtonElement) &&
!(innerRef.current instanceof HTMLAnchorElement)
) {
// eslint-disable-next-line no-console
console.error(
'Error: Found `Link` component that renders an inaccessible element',
innerRef.current,
'Please ensure `Link` always renders as <a> or <button>',
)
}
}, [innerRef])
}

return (
<StyledLink
as={Component}
{...props}
// @ts-ignore shh
ref={innerRef}
Expand Down
15 changes: 14 additions & 1 deletion src/Link/__tests__/Link.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {render as HTMLRender} from '@testing-library/react'
import {axe} from 'jest-axe'

describe('Link', () => {
behavesAsComponent({Component: Link, options: {skipAs: true}})
behavesAsComponent({Component: Link})

checkExports('Link', {
default: Link,
Expand All @@ -29,11 +29,24 @@ describe('Link', () => {
expect(render(<Link sx={{fontStyle: 'italic'}} />)).toHaveStyleRule('font-style', 'italic')
})

it('applies button styles when rendering a button element', () => {
expect(render(<Link as="button" />)).toMatchSnapshot()
})

it('respects the "muted" prop', () => {
expect(render(<Link muted />)).toMatchSnapshot()
})

it('respects the "sx" prop when "muted" prop is also passed', () => {
expect(render(<Link muted sx={{color: 'fg.onEmphasis'}} />)).toMatchSnapshot()
})

it('logs a warning when trying to render invalid "as" prop', () => {
const consoleSpy = jest.spyOn(global.console, 'error').mockImplementation()

HTMLRender(<Link as="i" />)
expect(consoleSpy).toHaveBeenCalled()

consoleSpy.mockRestore()
})
})
Loading