From d3a4c9e36b832eb4b321257ba14a0bd162ec7a3b Mon Sep 17 00:00:00 2001
From: Laura Cabrera <lzcabrera@gmail.com>
Date: Fri, 22 Sep 2017 13:12:37 -0700
Subject: [PATCH] refactor(icon): Add joinClassNames utility

And tweaked deprecation warnings
---
 src/components/Icon/Icon.jsx                  | 25 ++++++++++---------
 src/components/Icon/__tests__/Icon.spec.jsx   | 16 ++++++------
 .../__snapshots__/Icon.spec.jsx.snap          |  2 +-
 src/utils/__tests__/joinClassNames.spec.js    | 15 +++++++++++
 src/utils/joinClassNames.js                   |  3 +++
 5 files changed, 40 insertions(+), 21 deletions(-)
 create mode 100644 src/utils/__tests__/joinClassNames.spec.js
 create mode 100644 src/utils/joinClassNames.js

diff --git a/src/components/Icon/Icon.jsx b/src/components/Icon/Icon.jsx
index 8bd1298e0d..357ef545e8 100644
--- a/src/components/Icon/Icon.jsx
+++ b/src/components/Icon/Icon.jsx
@@ -3,6 +3,7 @@ import PropTypes from 'prop-types'
 
 import { deprecate } from '../../warn'
 import capitalize from '../../capitalize'
+import joinClassNames from '../../utils/joinClassNames'
 
 import styles from './Icon.modules.scss'
 
@@ -16,24 +17,24 @@ const Icon = ({ glyph, variant, fixedWidth, size, className, children, ...rest }
   }
 
   if (variant === 'disabled') {
-    deprecate('Icon', '\'disabled\' variant is deprecated.')
+    deprecate('Icon', 'The `disabled` variant is no longer supported. Use one of the supported variants instead.')
   }
 
   if (fixedWidth) {
-    deprecate('Icon', '\'fixedWidth\' prop is deprecated.')
+    deprecate('Icon', 'Creating `fixedWidth` icons is deprecated. Use `size` instead.')
   }
 
   if (children) {
-    deprecate('Icon', '\'children\' prop is deprecated.')
+    deprecate('Icon', 'Passing children components to Icon is deprecated. To add an accessible label use `aria-label`.')
   }
 
-  const classes = [
-    `${styles[`iconCore${capitalize(glyph)}`]}`,
-    `${variant ? styles[variant] : ''}`,
-    `${fixedWidth ? `${styles.fw}` : ''}`,
-    `${size ? `${styles[`size${size}`]}` : ''}`,
-    `${className ? `${className}` : ''}`
-  ].join(' ')
+  const classes = joinClassNames(
+    styles[`iconCore${capitalize(glyph)}`],
+    variant && styles[variant],
+    fixedWidth && styles.fw,
+    size && styles[`size${size}`],
+    className
+  )
 
   return (
     <i
@@ -91,7 +92,7 @@ Icon.propTypes = {
   /**
    * The icon size in pixels.
    */
-  size: PropTypes.oneOf(['16', '24', '48']),
+  size: PropTypes.oneOf([16, 24, 48]),
   /**
    * One or more CSS class names separated by spaces to append onto the icon.
    * Don't advertise as we plan on removing this feature soon.
@@ -107,7 +108,7 @@ Icon.propTypes = {
 Icon.defaultProps = {
   variant: 'inherit',
   fixedWidth: false,
-  size: '24',
+  size: 24,
   className: '',
   children: null
 }
diff --git a/src/components/Icon/__tests__/Icon.spec.jsx b/src/components/Icon/__tests__/Icon.spec.jsx
index c77dac2948..29394df628 100644
--- a/src/components/Icon/__tests__/Icon.spec.jsx
+++ b/src/components/Icon/__tests__/Icon.spec.jsx
@@ -41,7 +41,7 @@ describe('<Icon />', () => {
   })
 
   it('can be sized', () => {
-    const icon = doShallow({ size: '16' })
+    const icon = doShallow({ size: 16 })
 
     expect(icon).toHaveClassName('size16')
   })
@@ -60,7 +60,7 @@ describe('<Icon />', () => {
   })
 
   describe('deprecated props', () => {
-    afterEach(() => {
+    beforeEach(() => {
       jest.clearAllMocks()
     })
 
@@ -92,12 +92,12 @@ describe('<Icon />', () => {
       expect(deprecate).toHaveBeenCalled()
     })
 
-    // it('deprecates children prop', () => {
-    //   const defaultChildren = '<span>Some content</span>'
-    //   const icon = doShallow({ children: defaultChildren })
-    //
-    //   expect(deprecate).toHaveBeenCalled()
-    // })
+    it('deprecates children prop', () => {
+      const icon = doShallow({ children: 'Some content' })
+
+      expect(icon).toHaveText('Some content')
+      expect(deprecate).toHaveBeenCalled()
+    })
   })
 
   it('provides a label to specific glyphs', () => {
diff --git a/src/components/Icon/__tests__/__snapshots__/Icon.spec.jsx.snap b/src/components/Icon/__tests__/__snapshots__/Icon.spec.jsx.snap
index 79ab926aa2..ff23d49596 100644
--- a/src/components/Icon/__tests__/__snapshots__/Icon.spec.jsx.snap
+++ b/src/components/Icon/__tests__/__snapshots__/Icon.spec.jsx.snap
@@ -3,6 +3,6 @@
 exports[`<Icon /> renders 1`] = `
 <i
   aria-hidden="true"
-  className="iconCoreCheckmark inherit  size24 "
+  className="iconCoreCheckmark inherit size24"
 />
 `;
diff --git a/src/utils/__tests__/joinClassNames.spec.js b/src/utils/__tests__/joinClassNames.spec.js
new file mode 100644
index 0000000000..3f2ed0671d
--- /dev/null
+++ b/src/utils/__tests__/joinClassNames.spec.js
@@ -0,0 +1,15 @@
+import joinClassNames from '../joinClassNames'
+
+describe('joinClassNames', () => {
+  it('joins class names with a space in between', () => {
+    const classes = joinClassNames('class-1', 'class-2')
+
+    expect(classes).toEqual('class-1 class-2')
+  })
+
+  it('removes empties', () => {
+    const classes = joinClassNames('class-1', '', undefined, false, null, 'class-2')
+
+    expect(classes).toEqual('class-1 class-2')
+  })
+})
diff --git a/src/utils/joinClassNames.js b/src/utils/joinClassNames.js
new file mode 100644
index 0000000000..19f9f27e86
--- /dev/null
+++ b/src/utils/joinClassNames.js
@@ -0,0 +1,3 @@
+const joinClassNames = (...classes) => classes.filter(className => className).join(' ')
+
+export default joinClassNames