Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 8abef65

Browse filesBrowse files
khiga8khiga8
authored
Merge pull request #461 from github/kh-fix-bug-with-methods
Fix bugs with `getRole` and `getElementType`
2 parents 8385442 + 6896b71 commit 8abef65
Copy full SHA for 8abef65

File tree

Expand file treeCollapse file tree

6 files changed

+79
-11
lines changed
Filter options
Expand file treeCollapse file tree

6 files changed

+79
-11
lines changed

‎lib/utils/get-element-type.js

Copy file name to clipboardExpand all lines: lib/utils/get-element-type.js
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
const {elementType, getProp, getPropValue} = require('jsx-ast-utils')
1+
const {elementType, getProp, getLiteralPropValue} = require('jsx-ast-utils')
22

33
/*
44
Allows custom component to be mapped to an element type.
@@ -12,7 +12,7 @@ function getElementType(context, node, ignoreMap = false) {
1212

1313
// check if the node contains a polymorphic prop
1414
const polymorphicPropName = settings?.github?.polymorphicPropName ?? 'as'
15-
const rawElement = getPropValue(getProp(node.attributes, polymorphicPropName)) ?? elementType(node)
15+
const rawElement = getLiteralPropValue(getProp(node.attributes, polymorphicPropName)) ?? elementType(node)
1616

1717
// if a component configuration does not exists, return the raw element
1818
if (ignoreMap || !settings?.github?.components?.[rawElement]) return rawElement

‎lib/utils/get-role.js

Copy file name to clipboardExpand all lines: lib/utils/get-role.js
+3-3Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
const {getProp, getPropValue} = require('jsx-ast-utils')
1+
const {getProp, getLiteralPropValue} = require('jsx-ast-utils')
22
const {elementRoles} = require('aria-query')
33
const {getElementType} = require('./get-element-type')
44
const ObjectMap = require('./object-map')
@@ -43,7 +43,7 @@ function cleanElementRolesMap() {
4343
*/
4444
function getRole(context, node) {
4545
// Early return if role is explicitly set
46-
const explicitRole = getPropValue(getProp(node.attributes, 'role'))
46+
const explicitRole = getLiteralPropValue(getProp(node.attributes, 'role'))
4747
if (explicitRole) {
4848
return explicitRole
4949
}
@@ -80,7 +80,7 @@ function getRole(context, node) {
8080
continue
8181
}
8282

83-
const value = getPropValue(propOnNode)
83+
const value = getLiteralPropValue(propOnNode)
8484
if (value || (value === '' && prop === 'alt')) {
8585
if (
8686
prop === 'href' ||

‎tests/a11y-role-supports-aria-props.js

Copy file name to clipboardExpand all lines: tests/a11y-role-supports-aria-props.js
+11-3Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,9 @@ ruleTester.run('a11y-role-supports-aria-props', rule, {
3636
{code: '<div role="presentation" {...props} />'},
3737
{code: '<Foo.Bar baz={true} />'},
3838
{code: '<Link href="#" aria-checked />'},
39+
// Don't try to evaluate expression
40+
{code: '<Box aria-labelledby="some-id" role={role} />'},
41+
{code: '<Box aria-labelledby="some-id"as={isNavigationOpen ? "div" : "nav"} />'},
3942

4043
// IMPLICIT ROLE TESTS
4144
// A TESTS - implicit role is `link`
@@ -479,12 +482,17 @@ ruleTester.run('a11y-role-supports-aria-props', rule, {
479482
errors: [getErrorMessage('aria-labelledby', 'generic')],
480483
},
481484
{
482-
code: '<div aria-label />',
483-
errors: [getErrorMessage('aria-label', 'generic')],
485+
code: '<div aria-labelledby />',
486+
errors: [getErrorMessage('aria-labelledby', 'generic')],
484487
},
488+
// Determines role from literal `as` prop.
485489
{
486-
code: '<div aria-labelledby />',
490+
code: '<Box as="span" aria-labelledby />',
487491
errors: [getErrorMessage('aria-labelledby', 'generic')],
488492
},
493+
{
494+
code: '<p role="generic" aria-label />',
495+
errors: [getErrorMessage('aria-label', 'generic')],
496+
},
489497
],
490498
})

‎tests/utils/get-element-type.js

Copy file name to clipboardExpand all lines: tests/utils/get-element-type.js
+9-1Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
const {getElementType} = require('../../lib/utils/get-element-type')
2-
const {mockJSXAttribute, mockJSXOpeningElement} = require('./mocks')
2+
const {mockJSXAttribute, mockJSXConditionalAttribute, mockJSXOpeningElement} = require('./mocks')
33

44
const mocha = require('mocha')
55
const describe = mocha.describe
@@ -55,4 +55,12 @@ describe('getElementType', function () {
5555
const node = mockJSXOpeningElement('Link', [mockJSXAttribute('as', 'Button')])
5656
expect(getElementType(setting, node)).to.equal('button')
5757
})
58+
59+
it('returns raw type when polymorphic prop is set to non-literal expression', function () {
60+
// <Box as={isNavigationOpen ? 'generic' : 'navigation'} />
61+
const node = mockJSXOpeningElement('Box', [
62+
mockJSXConditionalAttribute('as', 'isNavigationOpen', 'generic', 'navigation'),
63+
])
64+
expect(getElementType({}, node)).to.equal('Box')
65+
})
5866
})

‎tests/utils/get-role.js

Copy file name to clipboardExpand all lines: tests/utils/get-role.js
+21-1Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,31 @@
11
const {getRole} = require('../../lib/utils/get-role')
2-
const {mockJSXAttribute, mockJSXOpeningElement} = require('./mocks')
2+
const {mockJSXAttribute, mockJSXConditionalAttribute, mockJSXOpeningElement} = require('./mocks')
33
const mocha = require('mocha')
44
const describe = mocha.describe
55
const it = mocha.it
66
const expect = require('chai').expect
77

88
describe('getRole', function () {
9+
it('returns undefined when polymorphic prop is set with a non-literal expression', function () {
10+
// <Box as={isNavigationOpen ? 'div' : 'nav'} />
11+
const node = mockJSXOpeningElement('Box', [mockJSXConditionalAttribute('as', 'isNavigationOpen', 'div', 'nav')])
12+
expect(getRole({}, node)).to.equal(undefined)
13+
})
14+
15+
it('returns undefined when role is set to non-literal expression', function () {
16+
// <Box role={isNavigationOpen ? 'generic' : 'navigation'} />
17+
const node = mockJSXOpeningElement('Box', [
18+
mockJSXConditionalAttribute('role', 'isNavigationOpen', 'generic', 'navigation'),
19+
])
20+
expect(getRole({}, node)).to.equal(undefined)
21+
})
22+
23+
it('returns `role` when set to a literal expression', function () {
24+
// <Box role="generic" />
25+
const node = mockJSXOpeningElement('Box', [mockJSXAttribute('role', 'generic')])
26+
expect(getRole({}, node)).to.equal('generic')
27+
})
28+
929
it('returns generic role for <span> regardless of attribute', function () {
1030
const node = mockJSXOpeningElement('span', [mockJSXAttribute('aria-label', 'something')])
1131
expect(getRole({}, node)).to.equal('generic')

‎tests/utils/mocks.js

Copy file name to clipboardExpand all lines: tests/utils/mocks.js
+33-1Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,38 @@ function mockJSXAttribute(prop, propValue) {
1212
}
1313
}
1414

15+
/* Mocks conditional expression
16+
e.g. <Box as={isNavigationOpen ? 'generic' : 'navigation'} /> can be by calling
17+
mockJSXConditionalAttribute('as', 'isNavigationOpen', 'generic', 'navigation')
18+
*/
19+
function mockJSXConditionalAttribute(prop, expression, consequentValue, alternateValue) {
20+
return {
21+
type: 'JSXAttribute',
22+
name: {
23+
type: 'JSXIdentifier',
24+
name: prop,
25+
},
26+
value: {
27+
type: 'JSXExpressionContainer',
28+
value: prop,
29+
expression: {
30+
type: 'ConditionalExpression',
31+
test: {
32+
type: expression,
33+
},
34+
consequent: {
35+
type: 'Literal',
36+
value: consequentValue,
37+
},
38+
alternate: {
39+
type: 'Literal',
40+
value: alternateValue,
41+
},
42+
},
43+
},
44+
}
45+
}
46+
1547
function mockJSXOpeningElement(tagName, attributes = []) {
1648
return {
1749
type: 'JSXOpeningElement',
@@ -23,4 +55,4 @@ function mockJSXOpeningElement(tagName, attributes = []) {
2355
}
2456
}
2557

26-
module.exports = {mockJSXAttribute, mockJSXOpeningElement}
58+
module.exports = {mockJSXAttribute, mockJSXOpeningElement, mockJSXConditionalAttribute}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.