Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

Add ButtonGroup component #33

Merged
merged 11 commits into from
Mar 8, 2018
4 changes: 2 additions & 2 deletions dist/toolbox.js

Large diffs are not rendered by default.

69 changes: 69 additions & 0 deletions src/components/ButtonGroup/ButtonGroup.jsx
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;

Copy link
Contributor

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';

const ButtonGroupContainer = glamorous.div({
display: 'inline-block',
});

const GroupedButton = glamorous(Button)({
Copy link
Contributor

Choose a reason for hiding this comment

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

I think ButtonGroupItem would probably be a better name here.

borderRadius: 0,

'&:first-of-type': {
borderTopLeftRadius: borderRadius,
borderBottomLeftRadius: borderRadius,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably handle border-width as well. We can do this by setting the border-right-width to 0 for all ButtonGroupItems except the last one (which would have a border-right-width of 1.

},
'&:last-of-type': {
borderTopRightRadius: borderRadius,
borderBottomRightRadius: borderRadius,
},
});

const ButtonGroup = ({
children,
className,
disabled,
onMouseEnter,
onMouseLeave,
}) => (
<ButtonGroupContainer
className={className}
onMouseEnter={onMouseEnter}
onMouseLeave={onMouseLeave}
Copy link
Contributor

@colebemis colebemis Mar 2, 2018

Choose a reason for hiding this comment

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

Do we need to explicitly pass onMouseEnter and onMouseLeave to the ButtonGroupContainer? I think a better approach might be to collect the unused props and spread them onto the container. Like this:

const ButtonGroup = ({ children, disabled, ...props }) => (
  <Div display="inline-block" {...props}>
    ...
  </Div>
);

Notice that I used Div instead of ButtonGroupContainer. Since ButtonGroupContainer only contains the display: inline-block rule, this is a good place to use glamorous' Div component and simply pass a display prop instead of defining a new component. From the glamorous docs:

Having to name all of that stuff could be tedious, so having these pre-built components is handy. The other handy bit here is that the props are the styles of these components.

I like to use the built-in components for layout purposes like setting display or position.

>
{React.Children.map(children, child => {
if (child.type !== Button) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 Button in a ButtonGroup?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 ButtonGroup component from breaking if it's passed components that aren't Buttons 👍

}
return <GroupedButton disabled={disabled} {...child.props} />;
})}
Copy link
Contributor

Choose a reason for hiding this comment

The 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:
return <ButtonGroupItem {...child.props} disabled={disabled || child.props.disabled} />;

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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),
Copy link
Contributor

Choose a reason for hiding this comment

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

I find that PropTypes read much nicer when you destructure the types in the import:

import { arrayOf, node } from 'prop-types';

...

ButtonGroup.propTypes = {
  children: arrayOf(node),
  ...
}

/** 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,
Copy link

Choose a reason for hiding this comment

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

children needs to go here too, this broke CI

Copy link
Contributor Author

@andrewjpiro andrewjpiro Mar 1, 2018

Choose a reason for hiding this comment

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

Yeah. This seems a bit odd to me because children is a prop on every component. There's discussion here on whether this should be enforced jsx-eslint/eslint-plugin-react#7 but I ended up adding it because it's not too difficult.

};

ButtonGroup.defaultProps = {
children: [],
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to have an empty ButtonGroup? I'm cool with making children a required prop.

className: '',
disabled: false,
onMouseEnter: () => {},
onMouseLeave: () => {},
Copy link

Choose a reason for hiding this comment

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

children needs a default prop too

};

export default ButtonGroup;
21 changes: 21 additions & 0 deletions src/components/ButtonGroup/ButtonGroup.test.jsx
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();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably add a test for when ButtonGroup wraps a component other than Button.

10 changes: 10 additions & 0 deletions src/components/ButtonGroup/README.md
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>
```
137 changes: 137 additions & 0 deletions src/components/ButtonGroup/__snapshots__/ButtonGroup.test.jsx.snap
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]}
/>
`;
1 change: 1 addition & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Contributor

Choose a reason for hiding this comment

The 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';
Expand Down