Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(Button): update handling of floated prop #1489

Merged
merged 2 commits into from
Mar 25, 2017
Merged

fix(Button): update handling of floated prop #1489

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f8b2b0a
fix(Button): update handling of floated prop
Mar 21, 2017
6adb5e6
fix(Button): update handling of floated prop
Mar 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 26 additions & 16 deletions src/elements/Button/Button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,15 +160,10 @@ class Button extends Component {
static Group = ButtonGroup
static Or = ButtonOr

handleClick = (e) => {
const { disabled, onClick } = this.props
computeElementType = () => {
const { attached, label } = this.props

if (disabled) {
e.preventDefault()
return
}

if (onClick) onClick(e, this.props)
if (!_.isNil(attached) || !_.isNil(label)) return 'div'
}

computeTabIndex = ElementType => {
Expand All @@ -179,6 +174,17 @@ class Button extends Component {
if (ElementType === 'div') return 0
}

handleClick = (e) => {
const { disabled, onClick } = this.props

if (disabled) {
e.preventDefault()
return
}

if (onClick) onClick(e, this.props)
}

render() {
const {
active,
Expand Down Expand Up @@ -207,10 +213,6 @@ class Button extends Component {
toggle,
} = this.props

const labeledClasses = cx(
useKeyOrValueAndKey(labelPosition || !!label, 'labeled'),
)

const baseClasses = cx(
color,
size,
Expand All @@ -229,18 +231,23 @@ class Button extends Component {
useKeyOnly(toggle, 'toggle'),
useKeyOrValueAndKey(animated, 'animated'),
useKeyOrValueAndKey(attached, 'attached'),
)
const labeledClasses = cx(
useKeyOrValueAndKey(labelPosition || !!label, 'labeled'),
)
const wrapperClasses = cx(
useKeyOnly(disabled, 'disabled'),
useValueAndKey(floated, 'floated'),
)
const wrapperClasses = cx(useKeyOnly(disabled, 'disabled'))

const rest = getUnhandledProps(Button, this.props)
const ElementType = getElementType(Button, this.props, () => {
if (!_.isNil(label) || !_.isNil(attached)) return 'div'
})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved this inline function to method, so it will not be created on each render.

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} tabIndex={tabIndex} onClick={this.handleClick}>
{children}
Expand All @@ -252,9 +259,11 @@ class Button extends Component {
basic: true,
pointing: labelPosition === 'left' ? 'right' : 'left',
})

if (labelElement) {
const classes = cx('ui', baseClasses, 'button', className)
const containerClasses = cx('ui', labeledClasses, 'button', className, wrapperClasses)

debug('render label:', { classes, containerClasses }, this.props)

return (
Expand All @@ -271,6 +280,7 @@ class Button extends Component {
if (!_.isNil(icon) && _.isNil(label)) {
const classes = cx('ui', labeledClasses, baseClasses, 'button', className, wrapperClasses)
debug('render icon && !label:', { classes })

return (
<ElementType {...rest} className={classes} tabIndex={tabIndex} onClick={this.handleClick}>
{Icon.create(icon)} {content}
Expand Down
5 changes: 5 additions & 0 deletions test/specs/elements/Button/Button-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,11 @@ describe('Button', () => {
wrapper.find('Label').should.not.have.className('disabled')
wrapper.find('button').should.not.have.className('disabled')
})
it('contains children without floated class when floated attribute is set', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would prefer we assert the expected location of the floated class in this test as well. It currently asserts where the className should not be, but doesn't assert where it should be.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated tests as you requested 👍

const wrapper = shallow(<Button label='hi' floated />)
wrapper.find('Label').should.not.have.className('floated')
wrapper.find('button').should.not.have.className('floated')
})
it('creates a basic pointing label', () => {
shallow(<Button label='foo' />)
.should.have.exactly(1).descendants('Label[basic][pointing]')
Expand Down