Skip to content

Commit 7bd36d2

Browse files
khiga8siddharthkp
khiga8
and
siddharthkp
authored
Introduce a new rule to flag Link in text blocks without inline (#183)
* feat: Flag link in text block * Cover newspace scenarios * Run lint * Passing tests, update configs * Cover scenario where Link has no parent with children * Fix false positives * additional test cases * All tests pass * Fix bug * Do not flag if only adjacent text is a dot * Fix deprecated notice * Add doc * Clean up * Fix bug * Fix format * Add test for underline * Skip font family * Update a11y-link-in-text-block.js * Update a11y-link-in-text-block.md * Run lint * Create happy-parents-invite.md * Update .changeset/happy-parents-invite.md Co-authored-by: Siddharth Kshetrapal <[email protected]> * Apply suggestions from code review --------- Co-authored-by: Siddharth Kshetrapal <[email protected]>
1 parent 9f9cfd2 commit 7bd36d2

File tree

5 files changed

+371
-0
lines changed

5 files changed

+371
-0
lines changed

‎.changeset/happy-parents-invite.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"eslint-plugin-primer-react": patch
3+
---
4+
5+
New rule to flag `Link` in text block missing the `inline` prop

‎docs/rules/a11y-link-in-text-block.md‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
## Require `inline` prop on `<Link>` component inside a text block
2+
3+
The `Link` component should have the `inline` prop when it is used inside of a text block.
4+
5+
## Rule Details
6+
7+
This rule enforces setting `inline` on the `<Link>` component when a `<Link>` is detected inside of a text block without distiguishable styling.
8+
9+
The lint rule will essentially flag any `<Link>` without the `inline` property (equal to `true`) detected with string nodes on either side.
10+
11+
This rule will not catch all instances of link in text block due to the limitations of static analysis, so be sure to also have in-browser checks in place such as the [link-in-text-block Axe rule](https://dequeuniversity.com/rules/axe/4.9/link-in-text-block) for additional coverage.
12+
13+
The edge cases that the linter skips to avoid false positives will include:
14+
15+
- `<Link sx={{fontWeight:...}}>` or `<Link sx={{fontFamily:...}}>` because these technically may provide sufficient distinguishing styling.
16+
- `<Link>` where the only adjacent text is a period, since that can't really be considered a text block.
17+
- `<Link>` where the children is a JSX component, rather than a string literal, because then it might be an icon link rather than a text link.
18+
- `<Link>` that are nested inside of headings as these have often been breadcrumbs.
19+
20+
👎 Examples of **incorrect** code for this rule
21+
22+
```jsx
23+
import {Link} from '@primer/react'
24+
25+
function ExampleComponent() {
26+
return (
27+
<SomeComponent>
28+
<Link>Say hello</Link> or not.
29+
</SomeComponent>
30+
)
31+
}
32+
```
33+
34+
```jsx
35+
import {Link} from '@primer/react'
36+
37+
function ExampleComponent() {
38+
return (
39+
<SomeComponent>
40+
Say hello or <Link>sign-up</Link>.
41+
</SomeComponent>
42+
)
43+
}
44+
```
45+
46+
👍 Examples of **correct** code for this rule:
47+
48+
```jsx
49+
function ExampleComponent() {
50+
return (
51+
<SomeComponent>
52+
<Link inline>Say hello</Link> or not.
53+
</SomeComponent>
54+
)
55+
}
56+
```
57+
58+
```jsx
59+
function ExampleComponent() {
60+
return (
61+
<SomeComponent>
62+
<Link inline={true}>Say hello</Link> or not.
63+
</SomeComponent>
64+
)
65+
}
66+
```
67+
68+
This rule will skip `Link`s containing JSX elements to minimize potential false positives because it is possible the JSX element sufficiently distinguishes the link from surrounding text.
69+
70+
```jsx
71+
function ExampleComponent() {
72+
return (
73+
<SomeComponent>
74+
<Link>
75+
<SomeAvatar />
76+
@monalisa
77+
</Link>{' '}
78+
commented on your account.
79+
</SomeComponent>
80+
)
81+
}
82+
```
83+
84+
This rule will skip `Link`s nested inside of a `Heading`.
85+
86+
```jsx
87+
function ExampleComponent() {
88+
return (
89+
<Heading>
90+
<Link>Previous location</Link>/ Current location
91+
</Heading>
92+
)
93+
}
94+
```
95+
96+
## Options
97+
98+
- `skipImportCheck` (default: `false`)
99+
100+
By default, the `a11y-explicit-heading` rule will only check for `<Heading>` components imported directly from `@primer/react`. You can disable this behavior by setting `skipImportCheck` to `true`.

‎src/index.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ module.exports = {
77
'new-color-css-vars': require('./rules/new-color-css-vars'),
88
'a11y-explicit-heading': require('./rules/a11y-explicit-heading'),
99
'no-deprecated-props': require('./rules/no-deprecated-props'),
10+
'a11y-link-in-text-block': require('./rules/a11y-link-in-text-block'),
1011
},
1112
configs: {
1213
recommended: require('./configs/recommended'),

‎src/rules/__tests__/a11y-link-in-text-block.test.js‎

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
const rule = require('../a11y-link-in-text-block')
2+
const {RuleTester} = require('eslint')
3+
4+
const ruleTester = new RuleTester({
5+
parserOptions: {
6+
ecmaVersion: 'latest',
7+
sourceType: 'module',
8+
ecmaFeatures: {
9+
jsx: true,
10+
},
11+
},
12+
})
13+
14+
ruleTester.run('a11y-link-in-text-block', rule, {
15+
valid: [
16+
`import {Link} from '@primer/react';
17+
<Box>
18+
19+
<Link href="something">
20+
Blah blah
21+
</Link>{' '}
22+
.
23+
</Box>
24+
`,
25+
`import {Text, Link} from '@primer/react';
26+
<Something>
27+
<Link href='blah'>
28+
blah
29+
</Link>
30+
</Something>
31+
`,
32+
`import {Link} from '@primer/react';
33+
<p>bla blah <Link inline={true}>Link level 1</Link></p>;
34+
`,
35+
`import {Link} from '@primer/react';
36+
<p>bla blah<Link inline>Link level 1</Link></p>;
37+
`,
38+
`import {Link} from '@primer/react';
39+
<><span>something</span><Link inline={true}>Link level 1</Link></>;
40+
`,
41+
`import {Link} from '@primer/react';
42+
<Link>Link level 1</Link>;
43+
`,
44+
`import {Heading, Link} from '@primer/react';
45+
<Heading>
46+
<Link>Link level 1</Link>
47+
hello
48+
</Heading>
49+
`,
50+
`import {Heading, Link} from '@primer/react';
51+
<Heading as="h2">
52+
<Link href={somePath}>
53+
Breadcrumb
54+
</Link>
55+
Create a thing
56+
</Heading>
57+
`,
58+
`import {Link} from '@primer/react';
59+
<div>
60+
<h2>
61+
<Link href={somePath}>
62+
Breadcrumb
63+
</Link>
64+
</h2>
65+
Create a thing
66+
</div>
67+
`,
68+
`import {Link} from '@primer/react';
69+
<div>
70+
<Link href={somePath}>
71+
<GitHubAvatar />{owner}
72+
</Link>{' '}
73+
last edited{' '}
74+
</div>
75+
`,
76+
`import {Link} from '@primer/react';
77+
<span>
78+
by
79+
<Link href="something" sx={{p: 2, fontWeight: 'bold'}}>
80+
Blah blah
81+
</Link>
82+
</span>
83+
`,
84+
`import {Link} from '@primer/react';
85+
<span>
86+
by
87+
<Link href="something" sx={{fontWeight: 'bold'}}>
88+
Blah blah
89+
</Link>
90+
</span>
91+
`,
92+
`import {Link} from '@primer/react';
93+
<span>
94+
by
95+
<Link href="something" sx={{fontFamily: 'mono'}}>
96+
Blah blah
97+
</Link>
98+
</span>
99+
`,
100+
`import {Link} from '@primer/react';
101+
<Box>
102+
103+
<Link href="something">
104+
Blah blah
105+
</Link>{' '}
106+
.
107+
</Box>
108+
`,
109+
`import {Link} from '@primer/react';
110+
<Heading sx={{fontSize: 1, mb: 3}} as="h3">
111+
In addition,{' '}
112+
<Link href="https://github.com/pricing" target="_blank">
113+
GitHub Team
114+
</Link>{' '}
115+
includes:
116+
</Heading>
117+
`,
118+
],
119+
invalid: [
120+
{
121+
code: `import {Link} from '@primer/react';
122+
<p>bla blah<Link>Link level 1</Link></p>
123+
`,
124+
errors: [{messageId: 'linkInTextBlock'}],
125+
},
126+
{
127+
code: `import {Link} from '@primer/react';
128+
<p><Link>Link level 1</Link> something something</p>
129+
`,
130+
errors: [{messageId: 'linkInTextBlock'}],
131+
},
132+
{
133+
code: `import {Link} from '@primer/react';
134+
<p>bla blah<Link inline={false}>Link level 1</Link></p>
135+
`,
136+
errors: [{messageId: 'linkInTextBlock'}],
137+
},
138+
{
139+
code: `import {Link} from '@primer/react';
140+
<Box>Something something{' '}
141+
<Link>Link level 1</Link>
142+
</Box>
143+
`,
144+
errors: [{messageId: 'linkInTextBlock'}],
145+
},
146+
{
147+
code: `import {Link} from '@primer/react';
148+
<>blah blah blah{' '}
149+
<Link>Link level 1</Link></>;
150+
`,
151+
errors: [{messageId: 'linkInTextBlock'}],
152+
},
153+
{
154+
code: `import {Link} from '@primer/react';
155+
<>blah blah blah{' '}
156+
<Link underline>Link level 1</Link></>;
157+
`,
158+
errors: [{messageId: 'linkInTextBlock'}],
159+
},
160+
],
161+
})

‎src/rules/a11y-link-in-text-block.js‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
const {isPrimerComponent} = require('../utils/is-primer-component')
2+
const {getJSXOpeningElementName} = require('../utils/get-jsx-opening-element-name')
3+
const {getJSXOpeningElementAttribute} = require('../utils/get-jsx-opening-element-attribute')
4+
5+
module.exports = {
6+
meta: {
7+
type: 'problem',
8+
schema: [
9+
{
10+
properties: {
11+
skipImportCheck: {
12+
type: 'boolean',
13+
},
14+
},
15+
},
16+
],
17+
messages: {
18+
linkInTextBlock: '<Link>s that are used within a text block should have the inline prop.',
19+
},
20+
},
21+
create(context) {
22+
const sourceCode = context.sourceCode ?? context.getSourceCode()
23+
return {
24+
JSXElement(node) {
25+
const name = getJSXOpeningElementName(node.openingElement)
26+
if (
27+
isPrimerComponent(node.openingElement.name, sourceCode.getScope(node)) &&
28+
name === 'Link' &&
29+
node.parent.children
30+
) {
31+
let siblings = node.parent.children
32+
const parentName = node.parent.openingElement?.name?.name
33+
// Skip if Link is nested inside of a heading.
34+
const parentsToSkip = ['Heading', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6']
35+
if (parentsToSkip.includes(parentName)) return
36+
if (siblings.length > 0) {
37+
siblings = siblings.filter(childNode => {
38+
return (
39+
!(childNode.type === 'JSXText' && /^\s+$/.test(childNode.value)) &&
40+
!(
41+
childNode.type === 'JSXExpressionContainer' &&
42+
childNode.expression.type === 'Literal' &&
43+
/^\s+$/.test(childNode.expression.value)
44+
) &&
45+
!(childNode.type === 'Literal' && /^\s+$/.test(childNode.value))
46+
)
47+
})
48+
const index = siblings.findIndex(childNode => {
49+
return childNode.range === node.range
50+
})
51+
const prevSibling = siblings[index - 1]
52+
const nextSibling = siblings[index + 1]
53+
54+
const prevSiblingIsText = prevSibling && prevSibling.type === 'JSXText'
55+
const nextSiblingIsText = nextSibling && nextSibling.type === 'JSXText'
56+
if (prevSiblingIsText || nextSiblingIsText) {
57+
// Skip if the only text adjacent to the link is a period, then skip it.
58+
if (!prevSiblingIsText && /^\s*\.+\s*$/.test(nextSibling.value)) {
59+
return
60+
}
61+
const sxAttribute = getJSXOpeningElementAttribute(node.openingElement, 'sx')
62+
const inlineAttribute = getJSXOpeningElementAttribute(node.openingElement, 'inline')
63+
64+
// Skip if Link child is a JSX element.
65+
const jsxElementChildren = node.children.filter(child => {
66+
return child.type === 'JSXElement'
67+
})
68+
if (jsxElementChildren.length > 0) return
69+
70+
// Skip if fontWeight or fontFamily is set via the sx prop since these may technically be considered sufficiently distinguishing styles that don't use color.
71+
if (
72+
sxAttribute &&
73+
sxAttribute?.value?.expression &&
74+
sxAttribute.value.expression.type === 'ObjectExpression' &&
75+
sxAttribute.value.expression.properties &&
76+
sxAttribute.value.expression.properties.length > 0
77+
) {
78+
const fontStyleProperty = sxAttribute.value.expression.properties.filter(property => {
79+
return property.key.name === 'fontWeight' || property.key.name === 'fontFamily'
80+
})
81+
if (fontStyleProperty.length > 0) return
82+
}
83+
if (inlineAttribute) {
84+
if (!inlineAttribute.value) {
85+
return
86+
} else if (inlineAttribute.value.type === 'JSXExpressionContainer') {
87+
if (inlineAttribute.value.expression.type === 'Literal') {
88+
if (inlineAttribute.value.expression.value === true) {
89+
return
90+
}
91+
}
92+
}
93+
}
94+
context.report({
95+
node,
96+
messageId: 'linkInTextBlock',
97+
})
98+
}
99+
}
100+
}
101+
},
102+
}
103+
},
104+
}

0 commit comments

Comments
 (0)