Skip to content

Commit d7cfe54

Browse files
jessebeachjessebeach
authored
Merge pull request #259 from mjaltamirano/label-has-for-update
update label-has-for children content handling
2 parents 6076fe5 + d22d480 commit d7cfe54

File tree

4 files changed

+94
-38
lines changed

4 files changed

+94
-38
lines changed

‎__tests__/src/rules/label-has-for-test.js

Lines changed: 57 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
// -----------------------------------------------------------------------------
1010

1111
import { RuleTester } from 'eslint';
12+
import assign from 'object.assign';
1213
import parserOptionsMapper from '../../__util__/parserOptionsMapper';
1314
import rule from '../../../src/rules/label-has-for';
1415

@@ -35,37 +36,48 @@ const optionsRequiredSome = [{
3536
const optionsRequiredEvery = [{
3637
required: { every: ['nesting', 'id'] },
3738
}];
39+
const optionsChildrenAllowed = [{
40+
allowChildren: true,
41+
}];
3842

3943
ruleTester.run('label-has-for', rule, {
4044
valid: [
4145
// DEFAULT ELEMENT 'label' TESTS
42-
{ code: '<label htmlFor="foo" />' },
43-
{ code: '<label htmlFor={"foo"} />' },
44-
{ code: '<label htmlFor={foo} />' },
45-
{ code: '<label htmlFor={`${id}`} />' },
4646
{ code: '<div />' },
47-
{ code: '<label htmlFor="foo">Test!</label>' },
47+
{ code: '<label htmlFor="foo"><input /></label>' },
4848
{ code: '<Label />' }, // lower-case convention refers to real HTML elements.
4949
{ code: '<Label htmlFor="foo" />' },
50+
{ code: '<Descriptor />' },
51+
{ code: '<Descriptor htmlFor="foo">Test!</Descriptor>' },
5052
{ code: '<UX.Layout>test</UX.Layout>' },
5153

5254
// CUSTOM ELEMENT ARRAY OPTION TESTS
53-
{ code: '<Label htmlFor="foo" />', options: array },
54-
{ code: '<Label htmlFor={"foo"} />', options: array },
55-
{ code: '<Label htmlFor={foo} />', options: array },
56-
{ code: '<Label htmlFor={`${id}`} />', options: array },
57-
{ code: '<div />', options: array },
58-
{ code: '<Label htmlFor="foo">Test!</Label>', options: array },
59-
{ code: '<Descriptor htmlFor="foo" />', options: array },
60-
{ code: '<Descriptor htmlFor={"foo"} />', options: array },
61-
{ code: '<Descriptor htmlFor={foo} />', options: array },
62-
{ code: '<Descriptor htmlFor={`${id}`} />', options: array },
55+
{ code: '<Label htmlFor="foo" />', options: [assign({}, array[0], optionsRequiredSome[0])] },
56+
{ code: '<Label htmlFor={"foo"} />', options: [assign({}, array[0], optionsRequiredSome[0])] },
57+
{ code: '<Label htmlFor={foo} />', options: [assign({}, array[0], optionsRequiredSome[0])] },
58+
{ code: '<Label htmlFor={`${id}`} />', options: [assign({}, array[0], optionsRequiredSome[0])] },
6359
{ code: '<div />', options: array },
64-
{ code: '<Descriptor htmlFor="foo">Test!</Descriptor>', options: array },
65-
{ code: '<label><input /></label>', options: optionsRequiredNesting },
66-
{ code: '<label><input /></label>', options: optionsRequiredSome },
60+
{ code: '<Label htmlFor="something"><input /></Label>', options: array },
61+
{ code: '<Label htmlFor="foo">Test!</Label>', options: [assign({}, array[0], optionsRequiredSome[0])] },
62+
{ code: '<Descriptor htmlFor="foo" />', options: [assign({}, array[0], optionsRequiredSome[0])] },
63+
{ code: '<Descriptor htmlFor={"foo"} />', options: [assign({}, array[0], optionsRequiredSome[0])] },
64+
{ code: '<Descriptor htmlFor={foo} />', options: [assign({}, array[0], optionsRequiredSome[0])] },
65+
{ code: '<Descriptor htmlFor={`${id}`} />', options: [assign({}, array[0], optionsRequiredSome[0])] },
66+
{ code: '<Descriptor htmlFor="foo">Test!</Descriptor>', options: [assign({}, array[0], optionsRequiredSome[0])] },
6767
{ code: '<label htmlFor="foo" />', options: optionsRequiredSome },
68-
{ code: '<label htmlFor="foo"><input /></label>', options: optionsRequiredEvery },
68+
{ code: '<label htmlFor={"foo"} />', options: optionsRequiredSome },
69+
{ code: '<label htmlFor={foo} />', options: optionsRequiredSome },
70+
{ code: '<label htmlFor={`${id}`} />', options: optionsRequiredSome },
71+
{ code: '<label htmlFor="foo">Test!</label>', options: optionsRequiredSome },
72+
{ code: '<label><input /></label>', options: optionsRequiredSome },
73+
{ code: '<label><input /></label>', options: optionsRequiredNesting },
74+
{ code: '<label htmlFor="input"><input /></label>', options: optionsRequiredEvery },
75+
{ code: '<label><input /></label>', options: optionsChildrenAllowed },
76+
{ code: '<Descriptor htmlFor="foo">Test!</Descriptor>', options: [assign({}, array, optionsChildrenAllowed)] },
77+
{ code: '<label>Test!</label>', options: optionsChildrenAllowed },
78+
{ code: '<label htmlFor="foo">Test!</label>', options: optionsChildrenAllowed },
79+
{ code: '<label>{children}</label>', options: optionsChildrenAllowed },
80+
{ code: '<label htmlFor="children">{children}</label>', options: optionsChildrenAllowed },
6981
].map(parserOptionsMapper),
7082
invalid: [
7183
// DEFAULT ELEMENT 'label' TESTS
@@ -75,8 +87,29 @@ ruleTester.run('label-has-for', rule, {
7587
{ code: '<label>First Name</label>', errors: [expectedError] },
7688
{ code: '<label {...props}>Foo</label>', errors: [expectedError] },
7789
{ code: '<label><input /></label>', errors: [expectedError] },
78-
79-
// CUSTOM ELEMENT ARRAY OPTION TESTS
90+
{ code: '<label>{children}</label>', errors: [expectedError] },
91+
{ code: '<label htmlFor="foo" />', errors: [expectedError] },
92+
{ code: '<label htmlFor={"foo"} />', errors: [expectedError] },
93+
{ code: '<label htmlFor={foo} />', errors: [expectedError] },
94+
{ code: '<label htmlFor={`${id}`} />', errors: [expectedError] },
95+
{ code: '<label htmlFor="foo">Test!</label>', errors: [expectedError] },
96+
//
97+
// // CUSTOM ELEMENT ARRAY OPTION TESTS
98+
{ code: '<Label></Label>', errors: [expectedError], options: array },
99+
{ code: '<Label htmlFor="foo" />', errors: [expectedError], options: array },
100+
{ code: '<Label htmlFor={"foo"} />', errors: [expectedError], options: array },
101+
{ code: '<Label htmlFor={foo} />', errors: [expectedError], options: array },
102+
{ code: '<Label htmlFor={`${id}`} />', errors: [expectedError], options: array },
103+
{ code: '<Label htmlFor="foo">Test!</Label>', errors: [expectedError], options: array },
104+
{ code: '<Descriptor htmlFor="foo" />', errors: [expectedError], options: array },
105+
{ code: '<Descriptor htmlFor={"foo"} />', errors: [expectedError], options: array },
106+
{ code: '<Descriptor htmlFor={foo} />', errors: [expectedError], options: array },
107+
{ code: '<Descriptor htmlFor={`${id}`} />', errors: [expectedError], options: array },
108+
{
109+
code: '<Descriptor htmlFor="foo">Test!</Descriptor>',
110+
errors: [expectedError],
111+
options: array,
112+
},
80113
{ code: '<Label id="foo" />', errors: [expectedError], options: array },
81114
{
82115
code: '<Label htmlFor={undefined} />',
@@ -115,11 +148,11 @@ ruleTester.run('label-has-for', rule, {
115148
errors: [expectedError],
116149
options: array,
117150
},
151+
{ code: '<label>{children}</label>', errors: [expectedError], options: array },
118152
{ code: '<label htmlFor="foo" />', errors: [expectedError], options: optionsRequiredNesting },
119153
{ code: '<label>First Name</label>', errors: [expectedError], options: optionsRequiredNesting },
120154
{ code: '<label>First Name</label>', errors: [expectedError], options: optionsRequiredSome },
121-
{ code: '<label htmlFor="foo" />', errors: [expectedError], options: optionsRequiredEvery },
122-
{ code: '<label><input /></label>', errors: [expectedError], options: optionsRequiredEvery },
123-
{ code: '<label>First Name</label>', errors: [expectedError], options: optionsRequiredEvery },
155+
{ code: '<label>{children}</label>', errors: [expectedError], options: optionsRequiredSome },
156+
{ code: '<label>{children}</label>', errors: [expectedError], options: optionsRequiredNesting },
124157
].map(parserOptionsMapper),
125158
});

‎docs/rules/label-has-for.md

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,8 @@ This rule takes one optional object argument of type object:
2020
"components": [ "Label" ],
2121
"required": {
2222
"every": [ "nesting", "id" ]
23-
}
23+
},
24+
"allowChildren": false,
2425
}],
2526
}
2627
}
@@ -53,14 +54,22 @@ return (
5354
);
5455
```
5556

56-
The `required` option (defaults to `"required": "id"`) determines which checks are activated. You're allowed to pass in one of the following types:
57+
The `required` option (defaults to `"required": { "every": ["nesting", "id"] }`) determines which checks are activated. You're allowed to pass in one of the following types:
5758

5859
- string: must be one of the acceptable strings (`"nesting"` or `"id"`)
5960
- object, must have one of the following properties:
6061

6162
- some: an array of acceptable strings, will pass if ANY of the requested checks passed
6263
- every: an array of acceptable strings, will pass if ALL of the requested checks passed
6364

65+
The `allowChildren` option (defaults to `false`) determines whether `{children}` content is allowed to be passed into a `label` element. For example, the following pattern, by default, is not allowed:
66+
67+
```js
68+
<label>{children}</label>
69+
```
70+
71+
However, if `allowChildren` is set to `true`, no error will be raised. If you want to pass in `{children}` content without raising an error, because you cannot be sure what `{children}` will render, then set `allowChildren` to `true`.
72+
6473
Note that passing props as spread attribute without `htmlFor` explicitly defined will cause this rule to fail. Explicitly pass down `htmlFor` prop for rule to pass. The prop must have an actual value to pass. Use `Label` component above as a reference. **It is a good thing to explicitly pass props that you expect to be passed for self-documentation.** For example:
6574

6675
#### Bad
@@ -90,8 +99,10 @@ function Foo(props) {
9099

91100
### Succeed
92101
```jsx
93-
<input type="text" id="firstName" />
94-
<label htmlFor="firstName">First Name</label>
102+
<label htmlFor="firstName">
103+
<input type="text" id="firstName" />
104+
First Name
105+
</label>
95106
```
96107

97108
### Fail

‎package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
"jest": "^20.0.4",
4949
"jscodeshift": "^0.3.30",
5050
"minimist": "^1.2.0",
51+
"object.assign": "^4.0.4",
5152
"rimraf": "^2.6.1",
5253
"to-ast": "^1.0.0"
5354
},

‎src/rules/label-has-for.js

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
import { getProp, getPropValue, elementType } from 'jsx-ast-utils';
1111
import { generateObjSchema, arraySchema, enumArraySchema } from '../util/schemas';
12+
import hasAccessibleChild from '../util/hasAccessibleChild';
1213

1314
const errorMessage = 'Form label must have associated control';
1415

@@ -24,28 +25,37 @@ const schema = {
2425
generateObjSchema({ every: enumArraySchema(enumValues) }, ['every']),
2526
],
2627
},
28+
allowChildren: { type: 'boolean' },
2729
},
2830
};
2931

30-
const validateNesting = node => !!node.parent.children.find(child => child.type === 'JSXElement');
32+
const validateNesting = node => node.parent.children.some(child => child.type === 'JSXElement');
33+
3134
const validateId = (node) => {
3235
const htmlForAttr = getProp(node.attributes, 'htmlFor');
3336
const htmlForValue = getPropValue(htmlForAttr);
3437

3538
return htmlForAttr !== false && !!htmlForValue;
3639
};
37-
const validate = (node, required) => (
38-
required === 'nesting' ? validateNesting(node) : validateId(node)
39-
);
4040

41-
const isValid = (node, required) => {
41+
const validate = (node, required, allowChildren) => {
42+
if (allowChildren === true) {
43+
return hasAccessibleChild(node.parent);
44+
}
45+
if (required === 'nesting') {
46+
return validateNesting(node);
47+
}
48+
return validateId(node);
49+
};
50+
51+
const isValid = (node, required, allowChildren) => {
4252
if (Array.isArray(required.some)) {
43-
return required.some.some(rule => validate(node, rule));
53+
return required.some.some(rule => validate(node, rule, allowChildren));
4454
} else if (Array.isArray(required.every)) {
45-
return required.every.every(rule => validate(node, rule));
55+
return required.every.every(rule => validate(node, rule, allowChildren));
4656
}
4757

48-
return validate(node, required);
58+
return validate(node, required, allowChildren);
4959
};
5060

5161
module.exports = {
@@ -66,9 +76,10 @@ module.exports = {
6676
return;
6777
}
6878

69-
const required = options.required || 'id';
79+
const required = options.required || { every: ['nesting', 'id'] };
80+
const allowChildren = options.allowChildren || false;
7081

71-
if (!isValid(node, required)) {
82+
if (!isValid(node, required, allowChildren)) {
7283
context.report({
7384
node,
7485
message: errorMessage,

0 commit comments

Comments
 (0)