feat(Button): add advanced handling of disabled prop (#1781)

Semantic-Org · Jun 19, 2017 · d8cafb1 · d8cafb1
1 parent 04ea612
commit d8cafb1
Show file tree

Hide file tree

Showing 2 changed files with 58 additions and 41 deletions.
diff --git a/src/elements/Button/Button.js b/src/elements/Button/Button.js
@@ -8,7 +8,6 @@ import {
   createShorthandFactory,
   getElementType,
   getUnhandledProps,
-  makeDebugger,
   META,
   SUI,
   useKeyOnly,
@@ -21,8 +20,6 @@ import ButtonContent from './ButtonContent'
 import ButtonGroup from './ButtonGroup'
 import ButtonOr from './ButtonOr'
 
-const debug = makeDebugger('button')
-
 /**
  * A Button indicates a possible user action.
  * @see Form
@@ -178,14 +175,14 @@ class Button extends Component {
   focus = () => _.invoke(this.ref, 'focus')
 
   handleClick = (e) => {
-    const { disabled, onClick } = this.props
+    const { disabled } = this.props
 
     if (disabled) {
       e.preventDefault()
       return
     }
 
-    if (onClick) onClick(e, this.props)
+    _.invoke(this.props, 'onClick', e, this.props)
   }
 
   handleRef = c => (this.ref = c)
@@ -256,56 +253,40 @@ class Button extends Component {
     const ElementType = getElementType(Button, this.props, this.computeElementType)
     const tabIndex = this.computeTabIndex(ElementType)
 
-    if (!_.isNil(children)) {
-      const classes = cx('ui', baseClasses, wrapperClasses, labeledClasses, 'button', className)
-      debug('render children:', { classes })
-
-      return (
-        <ElementType {...rest} className={classes} onClick={this.handleClick} ref={this.handleRef} tabIndex={tabIndex}>
-          {children}
-        </ElementType>
-      )
-    }
-
-    const labelElement = Label.create(label, { defaultProps: {
-      basic: true,
-      pointing: labelPosition === 'left' ? 'right' : 'left',
-    } })
-
-    if (labelElement) {
-      const classes = cx('ui', baseClasses, 'button', className)
+    if (!_.isNil(label)) {
+      const buttonClasses = cx('ui', baseClasses, 'button', className)
       const containerClasses = cx('ui', labeledClasses, 'button', className, wrapperClasses)
-
-      debug('render label:', { classes, containerClasses }, this.props)
+      const labelElement = Label.create(label, { defaultProps: {
+        basic: true,
+        pointing: labelPosition === 'left' ? 'right' : 'left',
+      } })
 
       return (
         <ElementType {...rest} className={containerClasses} onClick={this.handleClick}>
           {labelPosition === 'left' && labelElement}
-          <button className={classes} ref={this.handleRef} tabIndex={tabIndex}>
+          <button className={buttonClasses} disabled={disabled} ref={this.handleRef} tabIndex={tabIndex}>
             {Icon.create(icon)} {content}
           </button>
           {(labelPosition === 'right' || !labelPosition) && labelElement}
         </ElementType>
       )
     }
 
-    if (!_.isNil(icon) && _.isNil(label)) {
-      const classes = cx('ui', labeledClasses, baseClasses, 'button', className, wrapperClasses)
-      debug('render icon && !label:', { classes })
-
-      return (
-        <ElementType {...rest} className={classes} onClick={this.handleClick} ref={this.handleRef} tabIndex={tabIndex}>
-          {Icon.create(icon)} {content}
-        </ElementType>
-      )
-    }
-
-    const classes = cx('ui', labeledClasses, baseClasses, 'button', className, wrapperClasses)
-    debug('render default:', { classes })
+    const classes = cx('ui', baseClasses, wrapperClasses, labeledClasses, 'button', className)
+    const hasChildren = !_.isNil(children)
 
     return (
-      <ElementType {...rest} className={classes} onClick={this.handleClick} ref={this.handleRef} tabIndex={tabIndex}>
-        {content}
+      <ElementType
+        {...rest}
+        className={classes}
+        disabled={(disabled && ElementType === 'button') || undefined}
+        onClick={this.handleClick}
+        ref={this.handleRef}
+        tabIndex={tabIndex}
+      >
+        {hasChildren && children}
+        {!hasChildren && Icon.create(icon)}
+        {!hasChildren && content}
       </ElementType>
     )
   }

diff --git a/test/specs/elements/Button/Button-test.js b/test/specs/elements/Button/Button-test.js
@@ -71,6 +71,35 @@ describe('Button', () => {
     })
   })
 
+  describe('disabled', () => {
+    it('is not set by default', () => {
+      shallow(<Button />)
+        .should.not.have.prop('disabled')
+    })
+
+    it('applied when defined', () => {
+      shallow(<Button disabled />)
+        .should.have.prop('disabled', true)
+    })
+
+    it("don't apply when the element's type isn't button", () => {
+      shallow(<Button as='div' disabled />)
+        .should.not.have.prop('disabled')
+    })
+
+    it('is not set by default when has a label', () => {
+      shallow(<Button label='foo' />)
+        .find('button')
+        .should.not.have.prop('disabled')
+    })
+
+    it('applied when defined and has a label', () => {
+      shallow(<Button disabled label='foo' />)
+        .find('button')
+        .should.have.prop('disabled', true)
+    })
+  })
+
   describe('focus', () => {
     it('can be set via a ref', () => {
       const mountNode = document.createElement('div')
@@ -92,12 +121,19 @@ describe('Button', () => {
       shallow(<Button icon='user' />)
         .should.have.className('icon')
     })
+
+    it('adds className icon when true', () => {
+      shallow(<Button icon />)
+        .should.have.className('icon')
+    })
+
     it('does not add className icon when there is content', () => {
       shallow(<Button icon='user' content={0} />)
         .should.not.have.className('icon')
       shallow(<Button icon='user' content='Yo' />)
         .should.not.have.className('icon')
     })
+
     it('adds className icon given labelPosition and content', () => {
       shallow(<Button labelPosition='left' icon='user' content='My Account' />)
         .should.have.className('icon')