-
Notifications
You must be signed in to change notification settings - Fork 2
Add ButtonGroup component #33
Changes from 6 commits
87fc098
a3f7079
c81f839
0c2b0e6
323d8f5
a9a8478
a85c38b
c707efd
a1a264a
1dc8fa0
9f8493c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
import React from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import glamorous from 'glamorous'; | ||
|
||
import Button from '../Button/Button'; | ||
import constants from '../../constants'; | ||
|
||
const { borderRadius } = constants; | ||
|
||
const ButtonGroupContainer = glamorous.div({ | ||
display: 'inline-block', | ||
}); | ||
|
||
const GroupedButton = glamorous(Button)({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think |
||
borderRadius: 0, | ||
|
||
'&:first-of-type': { | ||
borderTopLeftRadius: borderRadius, | ||
borderBottomLeftRadius: borderRadius, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably handle |
||
}, | ||
'&:last-of-type': { | ||
borderTopRightRadius: borderRadius, | ||
borderBottomRightRadius: borderRadius, | ||
}, | ||
}); | ||
|
||
const ButtonGroup = ({ | ||
children, | ||
className, | ||
disabled, | ||
onMouseEnter, | ||
onMouseLeave, | ||
}) => ( | ||
<ButtonGroupContainer | ||
className={className} | ||
onMouseEnter={onMouseEnter} | ||
onMouseLeave={onMouseLeave} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to explicitly pass const ButtonGroup = ({ children, disabled, ...props }) => (
<Div display="inline-block" {...props}>
...
</Div>
); Notice that I used
I like to use the built-in components for layout purposes like setting |
||
> | ||
{React.Children.map(children, child => { | ||
if (child.type !== Button) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a little bit confused why this is necessary. Is there really a good reason to ever wrap something that isn't a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my use case I have a hidden input inside the ButtonGroup so there's one. |
||
return child; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did this because, in my use case, I wanted to put a hidden input inside of the ButtonGroup without it becoming a Button. I figured this was fine and it would be better not to screw with children that aren't buttons but maybe I'm wrong on that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was initially against this approach but after thinking about it for a little bit I think this is probably the right way to go. It'll prevent the |
||
} | ||
return <GroupedButton disabled={disabled} {...child.props} />; | ||
})} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't this mean that you can only ever disable the entire button group? What if we want to disable just a single button in the group? I think you could do something like: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch I'll fix that right away. |
||
</ButtonGroupContainer> | ||
); | ||
|
||
ButtonGroup.propTypes = { | ||
/** Buttons to be grouped together. */ | ||
children: PropTypes.arrayOf(PropTypes.node), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find that
|
||
/** Set class name of containing element. */ | ||
className: PropTypes.string, | ||
/** Indicates that the control is not available for interaction */ | ||
disabled: PropTypes.bool, | ||
/** Callback when mouse enters component. */ | ||
onMouseEnter: PropTypes.func, | ||
/** Callback when mouse leaves component. */ | ||
onMouseLeave: PropTypes.func, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. This seems a bit odd to me because |
||
}; | ||
|
||
ButtonGroup.defaultProps = { | ||
children: [], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it make sense to have an empty |
||
className: '', | ||
disabled: false, | ||
onMouseEnter: () => {}, | ||
onMouseLeave: () => {}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
}; | ||
|
||
export default ButtonGroup; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import React from 'react'; | ||
import renderer from 'react-test-renderer'; | ||
import ButtonGroup from './ButtonGroup'; | ||
import Button from '../Button/Button'; | ||
|
||
test('renders without crashing', () => { | ||
const tree = renderer.create(<ButtonGroup />).toJSON(); | ||
expect(tree).toMatchSnapshot(); | ||
}); | ||
|
||
test('renders when disabled', () => { | ||
const tree = renderer | ||
.create( | ||
<ButtonGroup disabled> | ||
<Button>Button 1</Button> | ||
<Button>Button 2</Button> | ||
</ButtonGroup>, | ||
) | ||
.toJSON(); | ||
expect(tree).toMatchSnapshot(); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably add a test for when |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
### Example | ||
|
||
``` | ||
<ButtonGroup> | ||
<Button>File</Button> | ||
<Button>Edit</Button> | ||
<Button>View</Button> | ||
<Button>History</Button> | ||
</ButtonGroup> | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,137 @@ | ||
// Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
||
exports[`renders when disabled 1`] = ` | ||
.glamor-6, | ||
[data-glamor-6] { | ||
display: inline-block; | ||
} | ||
|
||
.glamor-2, | ||
[data-glamor-2] { | ||
display: inline-block; | ||
box-sizing: border-box; | ||
font-family: inherit; | ||
text-decoration: none; | ||
border: 1px solid transparent; | ||
border-radius: 0; | ||
outline: 0; | ||
white-space: nowrap; | ||
cursor: pointer; | ||
user-select: none; | ||
transition: all 150ms ease-in-out; | ||
color: rgba(0, 3, 6, 0.87); | ||
background-color: transparent; | ||
border-color: rgba(0, 3, 6, 0.12); | ||
padding: 0.5rem 1rem; | ||
font-size: 0.875rem; | ||
line-height: 1.5; | ||
-webkit-user-select: none; | ||
-moz-user-select: none; | ||
-ms-user-select: none; | ||
-webkit-transition: all 150ms ease-in-out; | ||
-moz-transition: all 150ms ease-in-out; | ||
} | ||
|
||
.glamor-2:hover, | ||
[data-glamor-2]:hover { | ||
background-color: rgba(0, 3, 6, 0.04); | ||
} | ||
|
||
.glamor-2:focus, | ||
[data-glamor-2]:focus { | ||
box-shadow: 0 0 0 3px rgba(0, 3, 6, 0.12); | ||
} | ||
|
||
.glamor-2:first-of-type, | ||
[data-glamor-2]:first-of-type { | ||
border-top-left-radius: 4px; | ||
border-bottom-left-radius: 4px; | ||
} | ||
|
||
.glamor-2:last-of-type, | ||
[data-glamor-2]:last-of-type { | ||
border-top-right-radius: 4px; | ||
border-bottom-right-radius: 4px; | ||
} | ||
|
||
.glamor-1, | ||
[data-glamor-1] { | ||
width: 100%; | ||
display: -webkit-box; | ||
display: -moz-box; | ||
display: -ms-flexbox; | ||
display: -webkit-flex; | ||
display: flex; | ||
justify-content: center; | ||
align-items: center; | ||
text-align: center; | ||
-webkit-box-pack: center; | ||
-webkit-justify-content: center; | ||
-webkit-box-align: center; | ||
-webkit-align-items: center; | ||
} | ||
|
||
.glamor-1> :not(:first-child), | ||
[data-glamor-1]> :not(:first-child) { | ||
margin-left: 0.5rem; | ||
} | ||
|
||
.glamor-0, | ||
[data-glamor-0] { | ||
text-overflow: ellipsis; | ||
overflow: hidden; | ||
white-space: nowrap; | ||
} | ||
|
||
<div | ||
className="glamor-6" | ||
onMouseEnter={[Function]} | ||
onMouseLeave={[Function]} | ||
> | ||
<button | ||
className="glamor-2" | ||
disabled={false} | ||
onClick={[Function]} | ||
tabIndex={0} | ||
> | ||
<span | ||
className="glamor-1" | ||
> | ||
<span | ||
className="glamor-0" | ||
> | ||
Button 1 | ||
</span> | ||
</span> | ||
</button> | ||
<button | ||
className="glamor-2" | ||
disabled={false} | ||
onClick={[Function]} | ||
tabIndex={0} | ||
> | ||
<span | ||
className="glamor-1" | ||
> | ||
<span | ||
className="glamor-0" | ||
> | ||
Button 2 | ||
</span> | ||
</span> | ||
</button> | ||
</div> | ||
`; | ||
|
||
exports[`renders without crashing 1`] = ` | ||
.glamor-0, | ||
[data-glamor-0] { | ||
display: inline-block; | ||
} | ||
|
||
<div | ||
className="glamor-0" | ||
onMouseEnter={[Function]} | ||
onMouseLeave={[Function]} | ||
/> | ||
`; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import './plugins'; | |
export { default as glamorous } from 'glamorous'; | ||
|
||
export { default as Button } from './components/Button/Button'; | ||
export { default as ButtonGroup } from './components/ButtonGroup/ButtonGroup'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice 👍 I always forget to export components from Toolbox. |
||
export { default as Checkbox } from './components/Checkbox/Checkbox'; | ||
export { default as Grid } from './components/Grid/Grid'; | ||
export { default as Icon } from './components/Icon/Icon'; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could include this in the import above. It would just be:
import glamorous, { Div } from 'glamorous';