Skip to content

Commit

Permalink
Add: ESLint rule to avoid BaseControl component with label without an…
Browse files Browse the repository at this point in the history
… id. (#14151)
  • Loading branch information
jorgefilipecosta authored and aduth committed Apr 24, 2019
1 parent e5c1b8c commit 53b012a
Show file tree
Hide file tree
Showing 9 changed files with 177 additions and 24 deletions.
24 changes: 10 additions & 14 deletions packages/block-editor/src/components/color-palette/control.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
*/
import { BaseControl, ColorIndicator } from '@wordpress/components';
import { ifCondition, compose } from '@wordpress/compose';
import { Fragment } from '@wordpress/element';
import { sprintf, __ } from '@wordpress/i18n';

/**
Expand All @@ -27,22 +26,19 @@ export function ColorPaletteControl( {
const colorName = colorObject && colorObject.name;
const ariaLabel = sprintf( colorIndicatorAriaLabel, label.toLowerCase(), colorName || value );

const labelElement = (
<Fragment>
{ label }
{ value && (
<ColorIndicator
colorValue={ value }
aria-label={ ariaLabel }
/>
) }
</Fragment>
);

return (
<BaseControl
className="editor-color-palette-control block-editor-color-palette-control"
label={ labelElement }>
>
<BaseControl.VisualLabel>
{ label }
{ value && (
<ColorIndicator
colorValue={ value }
aria-label={ ariaLabel }
/>
) }
</BaseControl.VisualLabel>
<ColorPalette
className="editor-color-palette-control__color-palette block-editor-color-palette-control__color-palette"
value={ value }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,14 @@
exports[`ColorPaletteControl matches the snapshot 1`] = `
<BaseControl
className="editor-color-palette-control block-editor-color-palette-control"
label={
<React.Fragment>
Test Color
<ColorIndicator
aria-label="(current test color: red)"
colorValue="#f00"
/>
</React.Fragment>
}
>
<Component>
Test Color
<ColorIndicator
aria-label="(current test color: red)"
colorValue="#f00"
/>
</Component>
<WithColorContext(ColorPalette)
className="editor-color-palette-control__color-palette block-editor-color-palette-control__color-palette"
colors={
Expand Down
5 changes: 4 additions & 1 deletion packages/components/src/font-size-picker/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,10 @@ function FontSizePicker( {
const currentFontSizeName = ( currentFont && currentFont.name ) || ( ! value && _x( 'Normal', 'font size name' ) ) || _x( 'Custom', 'font size name' );

return (
<BaseControl label={ __( 'Font Size' ) }>
<BaseControl>
<BaseControl.VisualLabel>
{ __( 'Font Size' ) }
</BaseControl.VisualLabel>
<div className="components-font-size-picker__buttons">
{ ( fontSizes.length > 0 ) &&
<Dropdown
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
- New Rule: [`@wordpress/dependency-group`](https://github.com/WordPress/gutenberg/blob/master/packages/eslint-plugin/docs/rules/dependency-group.md)
- New Rule: [`@wordpress/valid-sprintf`](https://github.com/WordPress/gutenberg/blob/master/packages/eslint-plugin/docs/rules/valid-sprintf.md)
- New Rule: [`@wordpress/gutenberg-phase`](https://github.com/WordPress/gutenberg/blob/master/packages/eslint-plugin/docs/rules/gutenberg-phase.md)
- New Rule: [`@wordpress/no-base-control-with-label-without-id`](https://github.com/WordPress/gutenberg/blob/master/packages/eslint-plugin/docs/rules/no-base-control-with-label-without-id.md)

## 1.0.0 (2018-12-12)

Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ Rule|Description|Recommended
[no-unused-vars-before-return](/packages/eslint-plugin/docs/rules/no-unused-vars-before-return.md)|Disallow assigning variable values if unused before a return|✓
[react-no-unsafe-timeout](/packages/eslint-plugin/docs/rules/react-no-unsafe-timeout.md)|Disallow unsafe `setTimeout` in component|
[valid-sprintf](/packages/eslint-plugin/docs/rules/valid-sprintf.md)|Enforce valid sprintf usage|✓
[no-base-control-with-label-without-id](/packages/eslint-plugin/docs/rules/no-base-control-with-label-without-id.md)| Disallow the usage of BaseControl component with a label prop set but omitting the id property|✓

### Legacy

Expand Down
9 changes: 9 additions & 0 deletions packages/eslint-plugin/configs/custom.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ module.exports = {
'@wordpress/gutenberg-phase': 'error',
'@wordpress/no-unused-vars-before-return': 'error',
'@wordpress/valid-sprintf': 'error',
'@wordpress/no-base-control-with-label-without-id': 'error',
'no-restricted-syntax': [
'error',
{
Expand All @@ -23,6 +24,14 @@ module.exports = {
},
],
},
overrides: [
{
files: [ '*.native.js' ],
rules: {
'@wordpress/no-base-control-with-label-without-id': 'off',
},
},
],
settings: {
react: {
version: '16.6',
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
# Disallow the usage of BaseControl component with a label prop set but omitting the id property (no-base-control-with-label-without-id)

Base control component ideally should be used together with components providing user input. The label the BaseControl component receives, should be associated with some component providing user via an id attribute.
If a label is provided but the id is omitted it means that the developer missed the id prop or that BaseControl is not a good fit for the use case and a div together with a span can provide the same functionality.

## Rule details

Examples of **incorrect** code for this rule:

```jsx
<BaseControl
label="ok"
>
<input id="my-id" />
</BaseControl>
```


```jsx
<BaseControl label="ok" />
```

Examples of **correct** code for this rule:


```jsx
<BaseControl />
```

```jsx
<BaseControl
id="my-id"
>
<input id="my-id" />
</BaseControl>
```

```jsx
<BaseControl
label="ok"
id="my-id"
>
<input id="my-id" />
</BaseControl>
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/**
* External dependencies
*/
import { RuleTester } from 'eslint';

/**
* Internal dependencies
*/
import rule from '../no-base-control-with-label-without-id';

const ruleTester = new RuleTester( {
parserOptions: {
ecmaVersion: 6,
ecmaFeatures: {
jsx: true,
},
},
} );

ruleTester.run( 'no-base-control-with-label-without-id', rule, {
valid: [
{
code: `
<BaseControl
label="ok"
id="my-id"
/>`,
},
{
code: `<BaseControl />`,
},
{
code: `
<BaseControl
label="ok"
id="my-id"
>
<input id="my-id" />
</BaseControl>`,
},
{
code: `
<BaseControl>
<input id="my-id" />
</BaseControl>`,
},
{
code: `
<BaseControl
id="my-id"
>
<input id="my-id" />
</BaseControl>`,
},
],
invalid: [
{
code: `
<BaseControl
label="ok"
>
<input id="my-id" />
</BaseControl>`,
errors: [ { message: 'When using BaseControl component if a label property is passed an id property should also be passed.' } ],
},
{
code: `
<BaseControl
label="ok"
/>`,
errors: [ { message: 'When using BaseControl component if a label property is passed an id property should also be passed.' } ],
},
],
} );
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
module.exports = {
meta: {
type: 'problem',
schema: [],
},
create( context ) {
return {
'JSXOpeningElement[name.name=\'BaseControl\']': ( node ) => {
const containsAttribute = ( attrName ) => {
return node.attributes.some( ( attribute ) => {
return attribute.name.name === attrName;
} );
};
if (
containsAttribute( 'label' ) &&
! containsAttribute( 'id' )
) {
context.report( {
node,
message: 'When using BaseControl component if a label property is passed an id property should also be passed.',
} );
}
},
};
},
};

0 comments on commit 53b012a

Please sign in to comment.