From a0281e9f1b14d8f776c0fbbb038b9ca6e699b467 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Albert=20Juh=C3=A9=20Lluveras?= <contact@albertjuhe.com>
Date: Fri, 23 Aug 2019 18:35:34 +0200
Subject: [PATCH] Create OrderSelect and Label components (#905)

* Create Label and OrderSelect components

* Update review list item so it uses 'classnames'

* Split review order select styles from generic component

* Update snapshots

* Refactor Label so we don't need to check Symbol

* Add description to Label and OrderSelect components

* Use prop-types instead of prop-types-elementtype

* Simplify Label propTypes

* Update package-lock
---
 assets/js/base/components/label/index.js      | 63 ++++++++++++++
 .../label/test/__snapshots__/index.js.snap    | 62 ++++++++++++++
 assets/js/base/components/label/test/index.js | 85 +++++++++++++++++++
 .../base/components/load-more-button/index.js | 18 ++--
 .../js/base/components/order-select/index.js  | 63 ++++++++++++++
 .../base/components/order-select/style.scss   |  9 ++
 .../base/components/review-list-item/index.js | 17 ++--
 .../components/review-order-select/index.js   | 47 ++++------
 .../components/review-order-select/style.scss |  7 --
 package-lock.json                             | 11 +--
 10 files changed, 315 insertions(+), 67 deletions(-)
 create mode 100644 assets/js/base/components/label/index.js
 create mode 100644 assets/js/base/components/label/test/__snapshots__/index.js.snap
 create mode 100644 assets/js/base/components/label/test/index.js
 create mode 100644 assets/js/base/components/order-select/index.js
 create mode 100644 assets/js/base/components/order-select/style.scss

diff --git a/assets/js/base/components/label/index.js b/assets/js/base/components/label/index.js
new file mode 100644
index 00000000000..9079b225a91
--- /dev/null
+++ b/assets/js/base/components/label/index.js
@@ -0,0 +1,63 @@
+/**
+ * External dependencies
+ */
+import PropTypes from 'prop-types';
+import { Fragment } from 'react';
+import classNames from 'classnames';
+
+/**
+ * Component used to render an accessible text given a label and/or a
+ * screenReaderLabel. The wrapper element and wrapper props can also be
+ * specified via props.
+ */
+const Label = ( { label, screenReaderLabel, wrapperElement, wrapperProps } ) => {
+	let Wrapper;
+
+	if ( ! label && screenReaderLabel ) {
+		Wrapper = wrapperElement || 'span';
+		wrapperProps = {
+			...wrapperProps,
+			className: classNames( wrapperProps.className, 'screen-reader-text' ),
+		};
+
+		return (
+			<Wrapper { ...wrapperProps }>
+				{ screenReaderLabel }
+			</Wrapper>
+		);
+	}
+
+	Wrapper = wrapperElement || Fragment;
+
+	if ( label && screenReaderLabel && label !== screenReaderLabel ) {
+		return (
+			<Wrapper { ...wrapperProps }>
+				<span aria-hidden>
+					{ label }
+				</span>
+				<span className="screen-reader-text">
+					{ screenReaderLabel }
+				</span>
+			</Wrapper>
+		);
+	}
+
+	return (
+		<Wrapper { ...wrapperProps }>
+			{ label }
+		</Wrapper>
+	);
+};
+
+Label.propTypes = {
+	label: PropTypes.string,
+	screenReaderLabel: PropTypes.string,
+	wrapperElement: PropTypes.elementType,
+	wrapperProps: PropTypes.object,
+};
+
+Label.defaultProps = {
+	wrapperProps: {},
+};
+
+export default Label;
diff --git a/assets/js/base/components/label/test/__snapshots__/index.js.snap b/assets/js/base/components/label/test/__snapshots__/index.js.snap
new file mode 100644
index 00000000000..980093315ad
--- /dev/null
+++ b/assets/js/base/components/label/test/__snapshots__/index.js.snap
@@ -0,0 +1,62 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`Label with wrapperElement should render both label and screen reader label 1`] = `
+<label
+  className="foo-bar"
+  data-foo="bar"
+>
+  <span
+    aria-hidden={true}
+  >
+    Lorem
+  </span>
+  <span
+    className="screen-reader-text"
+  >
+    Ipsum
+  </span>
+</label>
+`;
+
+exports[`Label with wrapperElement should render only the label 1`] = `
+<label
+  className="foo-bar"
+  data-foo="bar"
+>
+  Lorem
+</label>
+`;
+
+exports[`Label with wrapperElement should render only the screen reader label 1`] = `
+<label
+  className="foo-bar screen-reader-text"
+  data-foo="bar"
+>
+  Ipsum
+</label>
+`;
+
+exports[`Label without wrapperElement should render both label and screen reader label 1`] = `
+Array [
+  <span
+    aria-hidden={true}
+  >
+    Lorem
+  </span>,
+  <span
+    className="screen-reader-text"
+  >
+    Ipsum
+  </span>,
+]
+`;
+
+exports[`Label without wrapperElement should render only the label 1`] = `"Lorem"`;
+
+exports[`Label without wrapperElement should render only the screen reader label 1`] = `
+<span
+  className="screen-reader-text"
+>
+  Ipsum
+</span>
+`;
diff --git a/assets/js/base/components/label/test/index.js b/assets/js/base/components/label/test/index.js
new file mode 100644
index 00000000000..bd573d051d8
--- /dev/null
+++ b/assets/js/base/components/label/test/index.js
@@ -0,0 +1,85 @@
+/**
+ * External dependencies
+ */
+import TestRenderer from 'react-test-renderer';
+
+/**
+ * Internal dependencies
+ */
+import Label from '../';
+
+describe( 'Label', () => {
+	describe( 'without wrapperElement', () => {
+		test( 'should render both label and screen reader label', () => {
+			const component = TestRenderer.create(
+				<Label label="Lorem" screenReaderLabel="Ipsum" />
+			);
+
+			expect( component.toJSON() ).toMatchSnapshot();
+		} );
+
+		test( 'should render only the label', () => {
+			const component = TestRenderer.create(
+				<Label label="Lorem" />
+			);
+
+			expect( component.toJSON() ).toMatchSnapshot();
+		} );
+
+		test( 'should render only the screen reader label', () => {
+			const component = TestRenderer.create(
+				<Label screenReaderLabel="Ipsum" />
+			);
+
+			expect( component.toJSON() ).toMatchSnapshot();
+		} );
+	} );
+
+	describe( 'with wrapperElement', () => {
+		test( 'should render both label and screen reader label', () => {
+			const component = TestRenderer.create(
+				<Label
+					label="Lorem"
+					screenReaderLabel="Ipsum"
+					wrapperElement="label"
+					wrapperProps={ {
+						className: 'foo-bar',
+						'data-foo': 'bar',
+					} }
+				/>
+			);
+
+			expect( component.toJSON() ).toMatchSnapshot();
+		} );
+
+		test( 'should render only the label', () => {
+			const component = TestRenderer.create(
+				<Label
+					label="Lorem"
+					wrapperElement="label"
+					wrapperProps={ {
+						className: 'foo-bar',
+						'data-foo': 'bar',
+					} }
+				/>
+			);
+
+			expect( component.toJSON() ).toMatchSnapshot();
+		} );
+
+		test( 'should render only the screen reader label', () => {
+			const component = TestRenderer.create(
+				<Label
+					screenReaderLabel="Ipsum"
+					wrapperElement="label"
+					wrapperProps={ {
+						className: 'foo-bar',
+						'data-foo': 'bar',
+					} }
+				/>
+			);
+
+			expect( component.toJSON() ).toMatchSnapshot();
+		} );
+	} );
+} );
diff --git a/assets/js/base/components/load-more-button/index.js b/assets/js/base/components/load-more-button/index.js
index f1eb708228a..eb52bb4b4cf 100644
--- a/assets/js/base/components/load-more-button/index.js
+++ b/assets/js/base/components/load-more-button/index.js
@@ -2,33 +2,25 @@
  * External dependencies
  */
 import { __ } from '@wordpress/i18n';
-import { Fragment } from 'react';
 import PropTypes from 'prop-types';
 
 /**
  * Internal dependencies
  */
+import Label from '../label';
 import './style.scss';
 
 export const LoadMoreButton = ( { onClick, label, screenReaderLabel } ) => {
-	const labelNode = ( screenReaderLabel && label !== screenReaderLabel ) ? (
-		<Fragment>
-			<span aria-hidden>
-				{ label }
-			</span>
-			<span className="screen-reader-text">
-				{ screenReaderLabel }
-			</span>
-		</Fragment>
-	) : label;
-
 	return (
 		<div className="wp-block-button wc-block-load-more">
 			<button
 				className="wp-block-button__link"
 				onClick={ onClick }
 			>
-				{ labelNode }
+				<Label
+					label={ label }
+					screenReaderLabel={ screenReaderLabel }
+				/>
 			</button>
 		</div>
 	);
diff --git a/assets/js/base/components/order-select/index.js b/assets/js/base/components/order-select/index.js
new file mode 100644
index 00000000000..051a7107727
--- /dev/null
+++ b/assets/js/base/components/order-select/index.js
@@ -0,0 +1,63 @@
+/**
+ * External dependencies
+ */
+import PropTypes from 'prop-types';
+import classNames from 'classnames';
+
+/**
+ * Internal dependencies
+ */
+import Label from '../label';
+import './style.scss';
+
+/**
+ * Component used for 'Order by' selectors, which renders a label
+ * and a <select> with the options provided in the props.
+ */
+const OrderSelect = ( { className, componentId, defaultValue, label, onChange, options, screenReaderLabel, readOnly, value } ) => {
+	const selectId = `wc-block-order-select__select-${ componentId }`;
+
+	return (
+		<p className={ classNames( 'wc-block-order-select', className ) }>
+			<Label
+				label={ label }
+				screenReaderLabel={ screenReaderLabel }
+				wrapperElement="label"
+				wrapperProps={ {
+					className: 'wc-block-order-select__label',
+					htmlFor: selectId,
+				} }
+			/>
+			<select // eslint-disable-line jsx-a11y/no-onchange
+				id={ selectId }
+				className="wc-block-order-select__select"
+				defaultValue={ defaultValue }
+				onChange={ onChange }
+				readOnly={ readOnly }
+				value={ value }
+			>
+				{ options.map( ( option ) => (
+					<option key={ option.key } value={ option.key }>
+						{ option.label }
+					</option>
+				) ) }
+			</select>
+		</p>
+	);
+};
+
+OrderSelect.propTypes = {
+	componentId: PropTypes.number.isRequired,
+	defaultValue: PropTypes.string,
+	label: PropTypes.string,
+	onChange: PropTypes.func,
+	options: PropTypes.arrayOf( PropTypes.shape( {
+		key: PropTypes.string.isRequired,
+		label: PropTypes.string.isRequired,
+	} ) ),
+	readOnly: PropTypes.bool,
+	screenReaderLabel: PropTypes.string,
+	value: PropTypes.string,
+};
+
+export default OrderSelect;
diff --git a/assets/js/base/components/order-select/style.scss b/assets/js/base/components/order-select/style.scss
new file mode 100644
index 00000000000..754eb573c66
--- /dev/null
+++ b/assets/js/base/components/order-select/style.scss
@@ -0,0 +1,9 @@
+.wc-block-order-select {
+	margin-bottom: $gap-small;
+}
+
+.wc-block-order-select__label {
+	margin-right: $gap-small;
+	display: inline-block;
+	font-weight: normal;
+}
diff --git a/assets/js/base/components/review-list-item/index.js b/assets/js/base/components/review-list-item/index.js
index 3f9521d711b..3e5a5129392 100644
--- a/assets/js/base/components/review-list-item/index.js
+++ b/assets/js/base/components/review-list-item/index.js
@@ -3,6 +3,7 @@
  */
 import { __, sprintf } from '@wordpress/i18n';
 import PropTypes from 'prop-types';
+import classNames from 'classnames';
 
 /**
  * Internal dependencies
@@ -10,16 +11,6 @@ import PropTypes from 'prop-types';
 import ReadMore from '../read-more';
 import './style.scss';
 
-function getReviewClasses( isLoading ) {
-	const classArray = [ 'wc-block-review-list-item__item' ];
-
-	if ( isLoading ) {
-		classArray.push( 'is-loading' );
-	}
-
-	return classArray.join( ' ' );
-}
-
 function getReviewImage( review, imageType, isLoading ) {
 	if ( isLoading || ! review ) {
 		return (
@@ -108,10 +99,12 @@ const ReviewListItem = ( { attributes, review = {} } ) => {
 	const { rating } = review;
 	const isLoading = ! Object.keys( review ).length > 0;
 	const showReviewRating = Number.isFinite( rating ) && showReviewRatingAttr;
-	const classes = getReviewClasses( isLoading );
 
 	return (
-		<li className={ classes } aria-hidden={ isLoading }>
+		<li
+			className={ classNames( 'wc-block-review-list-item__item', { 'is-loading': isLoading } ) }
+			aria-hidden={ isLoading }
+		>
 			{ ( showProductName || showReviewDate || showReviewerName || showReviewImage || showReviewRating ) && (
 				<div className="wc-block-review-list-item__info">
 					{ showReviewImage && getReviewImage( review, imageType, isLoading ) }
diff --git a/assets/js/base/components/review-order-select/index.js b/assets/js/base/components/review-order-select/index.js
index edac7a1566d..f55cca8f67b 100644
--- a/assets/js/base/components/review-order-select/index.js
+++ b/assets/js/base/components/review-order-select/index.js
@@ -7,45 +7,32 @@ import PropTypes from 'prop-types';
 /**
  * Internal dependencies
  */
+import OrderSelect from '../order-select';
 import './style.scss';
 
 const ReviewOrderSelect = ( { componentId, defaultValue, onChange, readOnly, value } ) => {
-	const selectId = `wc-block-review-order-select__select-${ componentId }`;
-
 	return (
-		<p className="wc-block-review-order-select">
-			<label className="wc-block-review-order-select__label" htmlFor={ selectId }>
-				<span aria-hidden>
-					{ __( 'Order by', 'woo-gutenberg-products-block' ) }
-				</span>
-				<span className="screen-reader-text">
-					{ __( 'Order reviews by', 'woo-gutenberg-products-block' ) }
-				</span>
-			</label>
-			<select // eslint-disable-line jsx-a11y/no-onchange
-				id={ selectId }
-				className="wc-block-review-order-select__select"
-				defaultValue={ defaultValue }
-				onChange={ onChange }
-				readOnly={ readOnly }
-				value={ value }
-			>
-				<option value="most-recent">
-					{ __( 'Most recent', 'woo-gutenberg-products-block' ) }
-				</option>
-				<option value="highest-rating">
-					{ __( 'Highest rating', 'woo-gutenberg-products-block' ) }
-				</option>
-				<option value="lowest-rating">
-					{ __( 'Lowest rating', 'woo-gutenberg-products-block' ) }
-				</option>
-			</select>
-		</p>
+		<OrderSelect
+			className="wc-block-review-order-select"
+			componentId={ componentId }
+			defaultValue={ defaultValue }
+			label={ __( 'Order by', 'woo-gutenberg-products-block' ) }
+			onChange={ onChange }
+			options={ [
+				{ key: 'most-recent', label: __( 'Most recent', 'woo-gutenberg-products-block' ) },
+				{ key: 'highest-rating', label: __( 'Highest rating', 'woo-gutenberg-products-block' ) },
+				{ key: 'lowest-rating', label: __( 'Lowest rating', 'woo-gutenberg-products-block' ) },
+			] }
+			readOnly={ readOnly }
+			screenReaderLabel={ __( 'Order reviews by', 'woo-gutenberg-products-block' ) }
+			value={ value }
+		/>
 	);
 };
 
 ReviewOrderSelect.propTypes = {
 	componentId: PropTypes.number.isRequired,
+	defaultValue: PropTypes.oneOf( [ 'most-recent', 'highest-rating', 'lowest-rating' ] ),
 	onChange: PropTypes.func,
 	readOnly: PropTypes.bool,
 	value: PropTypes.oneOf( [ 'most-recent', 'highest-rating', 'lowest-rating' ] ),
diff --git a/assets/js/base/components/review-order-select/style.scss b/assets/js/base/components/review-order-select/style.scss
index be5e57584f3..821a78bfb5e 100644
--- a/assets/js/base/components/review-order-select/style.scss
+++ b/assets/js/base/components/review-order-select/style.scss
@@ -1,10 +1,3 @@
 .wc-block-review-order-select {
-	margin-bottom: $gap-small;
 	text-align: right;
 }
-
-.wc-block-review-order-select__label {
-	margin-right: $gap-small;
-	display: inline-block;
-	font-weight: normal;
-}
diff --git a/package-lock.json b/package-lock.json
index af26f4062f4..d6c41e60027 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -18323,12 +18323,13 @@
       }
     },
     "prop-types": {
-      "version": "15.6.2",
-      "resolved": "https://registry.npmjs.org/prop-types/-/prop-types-15.6.2.tgz",
-      "integrity": "sha512-3pboPvLiWD7dkI3qf3KbUe6hKFKa52w+AE0VCqECtf+QHAKgOL37tTaNCnuX1nAAQ4ZhyP+kYVKf8rLmJ/feDQ==",
+      "version": "15.7.2",
+      "resolved": "https://registry.npmjs.org/prop-types/-/prop-types-15.7.2.tgz",
+      "integrity": "sha512-8QQikdH7//R2vurIJSutZ1smHYTcLpRWEOlHnzcWHmBYrOGUysKwSsrC89BCiFj3CbrfJ/nXFdJepOVrY1GCHQ==",
       "requires": {
-        "loose-envify": "^1.3.1",
-        "object-assign": "^4.1.1"
+        "loose-envify": "^1.4.0",
+        "object-assign": "^4.1.1",
+        "react-is": "^16.8.1"
       }
     },
     "prop-types-exact": {