Added the ability to specify label components that require a label attribute #668
Conversation
|
I think that this is a valid addition to the rule, I just don't feel happy about the API, I suggest the following as an alternative: I edited the test file so that if satisfied, the API would be working correctly (I think I did it correctly, but I reserve the right of being mistaken). New Test FilemakeRuleTester("form-control-has-label", rule, {
valid: [
"<label for=''><input type='text' /></label>",
"<input type='text' aria-label='test' />",
"<label for=''>text</label><input type='text' />",
"<input type='button'>",
`
<label>
<div>
<input type="radio" />
</div>
<div>
<slot />
</div>
</label>
`,
`
<div aria-hidden="true">
<input value="1" type="text" />
</div>
`,
{
code: "<custom-label for='input'>text</custom-label><input type='text' id='input' />",
options: [{ labelComponents: ["CustomLabel"] }]
},
{
code: "<custom-label label='text'><input type='text' id='input' /></custom-label>",
options: [{
labelComponents: [
{ name: "CustomLabel", requiredProps: ["label"] },
],
}]
},
{
code: `
<custom-label label='text'><input type='text' id='input' /></custom-label>
<custom-label-other><input type='text' id='input' /></custom-label-other>
`,
options: [{
labelComponents: [
{ name: "CustomLabel", requiredProps: ["label"] },
"CustomLabelOther",
],
}]
},
{
code: "<custom-label label='text' id='id' for='bla'><input type='text' id='input' /></custom-label>",
options: [{
labelComponents: [
{ name: "CustomLabel", requiredProps: ["label", "id", "for"] },
],
}]
},
{
code: "<custom-label><input type='text' id='input' /></custom-label>",
options: [{ labelComponents: ["CustomLabel"] }]
},
"<b-form-input />"
],
invalid: [
"<input type='text' />",
"<textarea type='text'></textarea>",
"<custom-label for='input'>text</custom-label><input type='text' id='input' />",
{
code: "<custom-label><input type='text' id='input' /></custom-label>",
options: [{
labelComponents: [
{ name: "CustomLabel", requiredProps: ["label"] },
],
}],
errors: [{ messageId: "default" }]
},
{
code: "<div><b-form-input /></div>",
options: [{ controlComponents: ["b-form-input"] }],
errors: [{ messageId: "default" }]
},
{
code: "<div label='text'><b-form-input /></div>",
options: [{ controlComponents: ["b-form-input"] }],
errors: [{ messageId: "default" }]
},
{
code: "<custom-label label='text'>label next to input</custom-label><input type='text' id='input' />",
options: [{ labelComponents: ["CustomLabel"] }],
errors: [{ messageId: "default" }]
},
{
code: "<label>label next to input</label><input type='text' id='input' />",
errors: [{ messageId: "default" }]
},
]
});I don't like your API suggestion because it is quite specific, not flexible, and it would be kinda clumsy to develop upon it if you deem necessary to expand the responsibilities of this rule. Plus this new API I'm suggesting covers more cases where the custom component would need some more props to properly define the relationship between the label and the control, like an |
|
Also, can you create an issue to pair with this PR, it's easier to manage the history of contributions that way, thanks :D |
|
Hi there! Thank you for reviewing the pull request :) With my limited knowledge of typescript, I did not manage to use a single config variable for both objects and strings (seems like that is anti-type behaviour?). So I kept two configuration options, with your requested changes in mind: I also created an issue and updated the description: #683 Lastly I noticed that the last two tests you created actually do not fail on this branch, nor the current release. But as this bahaviour is not changed, I do not think it is related to PR and excluded them. Should this be a separate issue? {
code: "<custom-label label='text'>label next to input</custom-label><input type='text' id='input' />",
options: [{ labelComponents: ["CustomLabel"] }],
errors: [{ messageId: "default" }]
},
{
code: "<label>label next to input</label><input type='text' id='input' />",
errors: [{ messageId: "default" }]
}, |
|
sorry for the year delay. I don't think that in this case it is an anti-pattern because this is directed to ease of use from the user of this library in their configuration, I always consider that the end-user is more important than pattern rules we create, and in this case I think it's ok. To define a type that can be one of two kinds in typescript you can use the pipe ( interface FormControlHasLabelOptions {
labelComponents: (string | LabelComponentsRequiredAttributes)[];
}about the // Valid configuration:
// "someRule": ["error", "strict"]
// "someRule": ["warn", { someNonOptionalProperty: true }]
// Invalid configuration:
// "someRule": "warn"
// "someRule": ["error"]
// "someRule": ["warn", { }]
// "someRule": ["error", "on"]
// "someRule": ["warn", { someOtherProperty: 5 }]
// "someRule": ["warn", { someNonOptionalProperty: false, someOtherProperty: 5 }]
module.exports = {
meta: {
schema: {
type: "array",
items: {
anyOf: [
{
type: "object",
properties: {
someNonOptionalProperty: { type: "boolean" }
},
required: ["someNonOptionalProperty"],
additionalProperties: false
},
{
enum: ["off", "strict"]
}
]
}
}
}
}Reference: https://eslint.org/docs/latest/extend/custom-rules#options-schemas |
Fixes #683
Vue bootstrap makes use of a parent component that requires a
labelattribute before it actually has a label. This PR allows to specifically check for this use case:https://bootstrap-vue.org/docs/components/form-group#label