From 3bc38751185e3a4818a540f5dce4c4195a571aed Mon Sep 17 00:00:00 2001
From: Jorge <jorge.costa@developer.pt>
Date: Wed, 27 Feb 2019 18:45:10 +0000
Subject: [PATCH] Add: ESLint rule to avoid BaseControl component with label
 without an id.

---
 .../src/components/color-palette/control.js   | 24 +++---
 .../test/__snapshots__/control.js.snap        | 16 ++--
 .../components/src/font-size-picker/index.js  |  5 +-
 packages/eslint-plugin/CHANGELOG.md           |  1 +
 packages/eslint-plugin/README.md              |  1 +
 packages/eslint-plugin/configs/custom.js      |  9 +++
 .../no-base-control-with-label-without-id.md  | 45 +++++++++++
 .../no-base-control-with-label-without-id.js  | 74 +++++++++++++++++++
 .../no-base-control-with-label-without-id.js  | 26 +++++++
 9 files changed, 177 insertions(+), 24 deletions(-)
 create mode 100644 packages/eslint-plugin/docs/rules/no-base-control-with-label-without-id.md
 create mode 100644 packages/eslint-plugin/rules/__tests__/no-base-control-with-label-without-id.js
 create mode 100644 packages/eslint-plugin/rules/no-base-control-with-label-without-id.js

diff --git a/packages/block-editor/src/components/color-palette/control.js b/packages/block-editor/src/components/color-palette/control.js
index 097cae030e9a3f..6a2da23b527c67 100644
--- a/packages/block-editor/src/components/color-palette/control.js
+++ b/packages/block-editor/src/components/color-palette/control.js
@@ -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';
 
 /**
@@ -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 }
diff --git a/packages/block-editor/src/components/color-palette/test/__snapshots__/control.js.snap b/packages/block-editor/src/components/color-palette/test/__snapshots__/control.js.snap
index 5d46ddc7277ef2..e49d5f33ee4e37 100644
--- a/packages/block-editor/src/components/color-palette/test/__snapshots__/control.js.snap
+++ b/packages/block-editor/src/components/color-palette/test/__snapshots__/control.js.snap
@@ -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={
diff --git a/packages/components/src/font-size-picker/index.js b/packages/components/src/font-size-picker/index.js
index 79f18faad4947f..6f6c22e12ff017 100644
--- a/packages/components/src/font-size-picker/index.js
+++ b/packages/components/src/font-size-picker/index.js
@@ -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
diff --git a/packages/eslint-plugin/CHANGELOG.md b/packages/eslint-plugin/CHANGELOG.md
index b13a52316a882b..54aad682e42dc2 100644
--- a/packages/eslint-plugin/CHANGELOG.md
+++ b/packages/eslint-plugin/CHANGELOG.md
@@ -24,6 +24,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)
 
diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md
index 4516d123428d68..2c8ef189000c15 100644
--- a/packages/eslint-plugin/README.md
+++ b/packages/eslint-plugin/README.md
@@ -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
 
diff --git a/packages/eslint-plugin/configs/custom.js b/packages/eslint-plugin/configs/custom.js
index d11c4ef232190d..7a51d736c3f290 100644
--- a/packages/eslint-plugin/configs/custom.js
+++ b/packages/eslint-plugin/configs/custom.js
@@ -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',
 			{
@@ -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',
diff --git a/packages/eslint-plugin/docs/rules/no-base-control-with-label-without-id.md b/packages/eslint-plugin/docs/rules/no-base-control-with-label-without-id.md
new file mode 100644
index 00000000000000..4dea158a3be12b
--- /dev/null
+++ b/packages/eslint-plugin/docs/rules/no-base-control-with-label-without-id.md
@@ -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>
+```
diff --git a/packages/eslint-plugin/rules/__tests__/no-base-control-with-label-without-id.js b/packages/eslint-plugin/rules/__tests__/no-base-control-with-label-without-id.js
new file mode 100644
index 00000000000000..a03a3a2296f7b1
--- /dev/null
+++ b/packages/eslint-plugin/rules/__tests__/no-base-control-with-label-without-id.js
@@ -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.' } ],
+		},
+	],
+} );
diff --git a/packages/eslint-plugin/rules/no-base-control-with-label-without-id.js b/packages/eslint-plugin/rules/no-base-control-with-label-without-id.js
new file mode 100644
index 00000000000000..4fd516a52cc68a
--- /dev/null
+++ b/packages/eslint-plugin/rules/no-base-control-with-label-without-id.js
@@ -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.',
+					} );
+				}
+			},
+		};
+	},
+};