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.

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

Rework Button Group to not use borders #7933

Merged
merged 9 commits into from
Jan 25, 2016

Conversation

andycochran
Copy link
Contributor

This PR implements margin for the space between buttons, instead of faking it with borders. It also nixes the use of table display & floats, instead using inline-block.

Let me know if you have feedback.

Woot!
-Andy

Related issues: #7665, #7844

@gakimball gakimball added the scss label Jan 20, 2016
@andycochran
Copy link
Contributor Author

Also makes button group spacing look good on backgrounds not matching $body-background.

@andycochran
Copy link
Contributor Author

Remedies these border-radius isses:
screen shot 2016-01-20 at 12 39 01 pm
screen shot 2016-01-20 at 12 42 11 pm

@gakimball
Copy link
Contributor

Does font-size: 0 get around the extra whitespace created by display: inline-block?

@andycochran
Copy link
Contributor Author

Exactly. The buttons have a space character's width between them. If the font size is zero, that space disappears.

@gakimball
Copy link
Contributor

One piece of feedback, the sizing classes shouldn't print inside the button-group() mixin—they should stay in the CSS output. Otherwise you're tightly coupled to those sizing class names (and class chaining) when using the mixin.

@andycochran
Copy link
Contributor Author

D'oh! I'll work on that. Thanks, @gakimball.

@andycochran
Copy link
Contributor Author

@gakimball Take a look. You'll notice I redid the button group sizes like this:

@each $size, $value in $button-sizes {
  &.#{$size} #{$buttongroup-child-selector} { font-size: $value; }
}

This allows user-defined button sizes.

A similar method could be used for all button sizing. For instance, lines 197–199 in _button.scss could be… 

@each $size, $value in $button-sizes {
  &.#{$size} { font-size: $value; }
}

instead of… 

&.tiny     { font-size: map-get($button-sizes, tiny); }
&.small    { font-size: map-get($button-sizes, small); }
&.large    { font-size: map-get($button-sizes, large); }

Caveat: It adds a .default button size class.
Thoughts?

@gakimball
Copy link
Contributor

Definitely interested in that. You can remove default from the map on the fly using map-remove().

@each $size, $value in map-remove($button-sizes, default) {
}

@andycochran
Copy link
Contributor Author

@gakimball Done. This'll close #7665, #7844, and #7972.

@gakimball
Copy link
Contributor

@andycochran Solid, thanks so much for putting it together!

gakimball added a commit that referenced this pull request Jan 25, 2016
Rework Button Group to not use borders
@gakimball gakimball merged commit dea6e0a into foundation:develop Jan 25, 2016
@slavanga
Copy link
Contributor

The font-size: 0 trick doesn't work in the android browser pre Jellybean.
http://codepen.io/stowball/pen/LsICH

@andycochran
Copy link
Contributor Author

@slavanga We decided in #7665 (comment) that this was acceptable fallout in oder to fix a few other bugs in all browsers.

@slavanga
Copy link
Contributor

@andycochran Alright, that sounds reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants