Add ButtonGroup component #33

andrewjpiro · 2018-03-01T18:33:35Z

This adds a component for grouping Buttons together. It simply adds the
correct borderRadius to any of it's children that are buttons and passes
through the rest of the props. Along with this, the group can be
disabled rather than having to disable all buttons individually.

On Deploy

Choose "Squash and merge" instead of "Create a merge commit". Then, in the title field write:
"feat: Add ButtonGroup component".

This is so the commit messages don't end up cluttering the release notes.

This adds a component for grouping Buttons together. It simply adds the correct borderRadius to any of it's children that are buttons and passes through the rest of the props. Along with this, the group can be disabled rather than having to disable all buttons individually.

andrewjpiro · 2018-03-01T18:35:08Z

src/components/ButtonGroup/ButtonGroup.jsx

+   >
+      {React.Children.map(children, child => {
+         if (child.type !== Button) {
+            return child;


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.

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 👍

n-fisher · 2018-03-01T18:38:08Z

src/components/ButtonGroup/ButtonGroup.jsx

+   /** Callback when mouse enters component. */
+   onMouseEnter: PropTypes.func,
+   /** Callback when mouse leaves component. */
+   onMouseLeave: PropTypes.func,


children needs to go here too, this broke CI

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.

n-fisher · 2018-03-01T18:38:31Z

src/components/ButtonGroup/ButtonGroup.jsx

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


children needs a default prop too

n-fisher · 2018-03-01T18:40:00Z

src/components/ButtonGroup/ButtonGroup.jsx

+            <GroupedButton
+               disabled={disabled}
+               {...child.props}
+            />


could this fit in 80 chars? <GroupedButton disabled={disabled} {...child.props} />

CI complains here.

n-fisher · 2018-03-01T19:34:36Z

dev_block 🌮 merge conflicts but CR 👍 progress looks good

There are still CI failures. Hopefully this actually fixes them.

codecov · 2018-03-01T20:29:55Z

Codecov Report

Merging #33 into master will increase coverage by 24.23%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           master      #33       +/-   ##
===========================================
+ Coverage   19.71%   43.94%   +24.23%     
===========================================
  Files          14       16        +2     
  Lines         142      157       +15     
  Branches       11       14        +3     
===========================================
+ Hits           28       69       +41     
+ Misses        114       88       -26

Impacted Files	Coverage Δ
src/components/ButtonGroup/ButtonGroup.jsx	`100% <100%> (ø)`
src/components/SearchInput/SearchInput.jsx	`100% <0%> (ø)`	⬆️
src/components/Avatar/Avatar.jsx	`100% <0%> (ø)`
src/components/Button/Button.jsx	`100% <0%> (+100%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed707e6...9f8493c. Read the comment docs.

This adds new tests for the ButtonGroup with snapshots. It also pushes the latest version of the toolbox distribution since that got borked during merging.

n-fisher · 2018-03-01T21:36:41Z

good to un block?

andrewjpiro · 2018-03-01T23:48:53Z

Yep. un_dev_block 💥

n-fisher · 2018-03-01T23:55:11Z

CR 👍

colebemis · 2018-03-02T00:50:38Z

src/components/ButtonGroup/ButtonGroup.jsx

+
+ButtonGroup.propTypes = {
+   /** Buttons to be grouped together. */
+   children: PropTypes.arrayOf(PropTypes.node),


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), ... }

colebemis · 2018-03-02T01:11:02Z

src/components/ButtonGroup/ButtonGroup.jsx

+   <ButtonGroupContainer
+      className={className}
+      onMouseEnter={onMouseEnter}
+      onMouseLeave={onMouseLeave}


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.

colebemis · 2018-03-02T01:15:21Z

src/components/ButtonGroup/ButtonGroup.jsx

+   >
+      {React.Children.map(children, child => {
+         if (child.type !== Button) {
+            return child;


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 👍

colebemis · 2018-03-02T01:16:13Z

src/components/ButtonGroup/ButtonGroup.jsx

+   display: 'inline-block',
+});
+
+const GroupedButton = glamorous(Button)({


I think ButtonGroupItem would probably be a better name here.

colebemis · 2018-03-02T01:18:41Z

src/components/ButtonGroup/ButtonGroup.jsx

+
+   '&:first-of-type': {
+      borderTopLeftRadius: borderRadius,
+      borderBottomLeftRadius: borderRadius,


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.

colebemis · 2018-03-02T01:20:11Z

src/components/ButtonGroup/ButtonGroup.test.jsx

+      )
+      .toJSON();
+   expect(tree).toMatchSnapshot();
+});


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

colebemis · 2018-03-02T01:21:09Z

src/components/ButtonGroup/ButtonGroup.jsx

+};
+
+ButtonGroup.defaultProps = {
+   children: [],


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

colebemis · 2018-03-02T01:21:57Z

src/index.js

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


Nice 👍 I always forget to export components from Toolbox.

colebemis · 2018-03-02T01:31:08Z

Nice work! Thanks for bearing with me as Toolbox changes :)

I know the commit message convention can be annoying (I'm not a huge fan either) but I promise that the benefits of formalized commit messages outweigh the annoyances. You can read more about that here: https://github.com/iFixit/toolbox#why

If you have any other feedback about how we can improve Toolbox, I'd love to hear it.

deploy_block 👍 on responding to my comments.

sterlinghirsh · 2018-03-02T18:46:06Z

CR ⚡ I've read to here but it's still blocked on some changes

addison-grant · 2018-03-02T19:22:58Z

Tested out the different props/methods on the example code for the ButtonGroup and its four children Buttons. The only one that didn't seem to have any effect was disabled on the ButtonGroup. screen shot 2018-03-02 at 11 19 43 am

dev_block ☀️

The issue was that the disabled prop was being overridden by the more generic {...child.props}. To fix this I just had to reorder the props. Along with this I made several style changes suggested in CR.

This fixes the issue of having both the borderLeft of one button and the borderRight of the other button creating a border with twice the normal width. Instead, we remove the borderRight for all but the last button and use only the borderLefts to separate buttons.

This adds the test case for when a ButtonGroup has other children besides Button components. It should, in this case, not alter the other children as the updated shapshots show.

andrewjpiro · 2018-03-06T02:07:54Z

Addressed all the comments with the last few commits.
Also checked the deploy preview and the disabled prop now works.

un_deploy_block 💥 un_dev_block 💥

lexahall · 2018-03-06T03:37:34Z

src/components/ButtonGroup/ButtonGroup.jsx

+
+const { borderRadius } = constants;
+const { Div } = glamorous;
+


You could include this in the import above. It would just be:
import glamorous, { Div } from 'glamorous';

lexahall · 2018-03-06T03:39:21Z

src/components/ButtonGroup/ButtonGroup.jsx

+const ButtonGroup = ({ children, disabled, ...props }) => (
+   <Div display="inline-block" {...props}>
+      {React.Children.map(children, child => {
+         if (child.type !== Button) {


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?

In my use case I have a hidden input inside the ButtonGroup so there's one.

lexahall · 2018-03-06T03:44:02Z

src/components/ButtonGroup/ButtonGroup.test.jsx

+         </ButtonGroup>,
+      )
+      .toJSON();
+


This is super minor, but I'd probably remove this extra line. Just to be consistent with how it is above.

lexahall · 2018-03-06T03:50:06Z

src/components/ButtonGroup/ButtonGroup.jsx

+            return child;
+         }
+         return <ButtonGroupItem {...child.props} disabled={disabled} />;
+      })}


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} />;

Good catch I'll fix that right away.

sterlinghirsh · 2018-03-06T18:28:07Z

I've read up to here now, but there are still some outstanding questions

The fact that the ButtonGroup's disabled overrode the childrens meant that you couldn't individually disable the children. This updates it so you still can and adds a test case to verify it works.

This removes the unnecessary additional line for destructuring glamorous to get Div.

andrewjpiro · 2018-03-06T20:01:14Z

Okay I think I've addressed everything. Another round!

lexahall · 2018-03-06T20:21:36Z

Thanks for making those changes!
CR

sterlinghirsh · 2018-03-06T20:27:43Z

CR ⚡

addison-grant · 2018-03-06T21:47:22Z

QA ☀️

Made sure the ButtonGroup component's disabled attribute trickled down to the children Buttons on the test page.
Made sure the children Buttons could have their optional props/methods set as before while part of a group.

andrewjpiro commented Mar 1, 2018

View reviewed changes

n-fisher reviewed Mar 1, 2018

View reviewed changes

Andrew Pirondini added 2 commits March 1, 2018 12:22

Add children PropType validation and export ButtonGroup

a3f7079

Make GroupedButton component fit on one line

c81f839

Andrew Pirondini added 2 commits March 1, 2018 12:45

Merge branch 'master' into add-buttongroup-component

0c2b0e6

style: Make GroupedButton on one line

323d8f5

There are still CI failures. Hopefully this actually fixes them.

test: Add missing ButtonGroup test

a9a8478

This adds new tests for the ButtonGroup with snapshots. It also pushes the latest version of the toolbox distribution since that got borked during merging.

colebemis reviewed Mar 2, 2018

View reviewed changes

addison-grant added the QAing label Mar 2, 2018

addison-grant removed the QAing label Mar 2, 2018

Andrew Pirondini added 3 commits March 5, 2018 18:50

fix(buttongroup): Fix disabled prop being overridden

a85c38b

The issue was that the disabled prop was being overridden by the more generic {...child.props}. To fix this I just had to reorder the props. Along with this I made several style changes suggested in CR.

test(buttongroup): Add test case for non-button children

a1a264a

This adds the test case for when a ButtonGroup has other children besides Button components. It should, in this case, not alter the other children as the updated shapshots show.

lexahall reviewed Mar 6, 2018

View reviewed changes

Andrew Pirondini added 2 commits March 6, 2018 12:52

fix(buttongroup): Fix bug preventing children from being disabled

1dc8fa0

The fact that the ButtonGroup's disabled overrode the childrens meant that you couldn't individually disable the children. This updates it so you still can and adds a test case to verify it works.

style(buttongroup): Fix glamorous import

9f8493c

This removes the unnecessary additional line for destructuring glamorous to get Div.

addison-grant added the QAing label Mar 6, 2018

addison-grant removed the QAing label Mar 6, 2018

andrewjpiro merged commit 060ab34 into master Mar 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ButtonGroup component #33

Add ButtonGroup component #33

andrewjpiro commented Mar 1, 2018 •

edited by lexahall

Loading

andrewjpiro Mar 1, 2018

colebemis Mar 2, 2018

n-fisher Mar 1, 2018

andrewjpiro Mar 1, 2018 •

edited

Loading

n-fisher Mar 1, 2018

n-fisher Mar 1, 2018

n-fisher commented Mar 1, 2018

codecov bot commented Mar 1, 2018 •

edited

Loading

n-fisher commented Mar 1, 2018

andrewjpiro commented Mar 1, 2018

n-fisher commented Mar 1, 2018

colebemis Mar 2, 2018

colebemis Mar 2, 2018 •

edited

Loading

colebemis Mar 2, 2018

colebemis Mar 2, 2018

colebemis Mar 2, 2018

colebemis Mar 2, 2018

colebemis Mar 2, 2018

colebemis Mar 2, 2018

colebemis commented Mar 2, 2018

sterlinghirsh commented Mar 2, 2018

addison-grant commented Mar 2, 2018

andrewjpiro commented Mar 6, 2018 •

edited

Loading

lexahall Mar 6, 2018

lexahall Mar 6, 2018

andrewjpiro Mar 6, 2018

lexahall Mar 6, 2018

lexahall Mar 6, 2018

andrewjpiro Mar 6, 2018

sterlinghirsh commented Mar 6, 2018

andrewjpiro commented Mar 6, 2018

lexahall commented Mar 6, 2018

sterlinghirsh commented Mar 6, 2018

addison-grant commented Mar 6, 2018


		const { borderRadius } = constants;
		const { Div } = glamorous;

Add ButtonGroup component #33

Add ButtonGroup component #33

Conversation

andrewjpiro commented Mar 1, 2018 • edited by lexahall Loading

On Deploy

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewjpiro Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n-fisher commented Mar 1, 2018

codecov bot commented Mar 1, 2018 • edited Loading

Codecov Report

n-fisher commented Mar 1, 2018

andrewjpiro commented Mar 1, 2018

n-fisher commented Mar 1, 2018

Choose a reason for hiding this comment

colebemis Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colebemis commented Mar 2, 2018

sterlinghirsh commented Mar 2, 2018

addison-grant commented Mar 2, 2018

andrewjpiro commented Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sterlinghirsh commented Mar 6, 2018

andrewjpiro commented Mar 6, 2018

lexahall commented Mar 6, 2018

sterlinghirsh commented Mar 6, 2018

addison-grant commented Mar 6, 2018

andrewjpiro commented Mar 1, 2018 •

edited by lexahall

Loading

andrewjpiro Mar 1, 2018 •

edited

Loading

codecov bot commented Mar 1, 2018 •

edited

Loading

colebemis Mar 2, 2018 •

edited

Loading

andrewjpiro commented Mar 6, 2018 •

edited

Loading