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

[Button group] radius doesn't wrap the whole group, but each one! #8447

Closed
abdullahsalem opened this issue Mar 22, 2016 · 13 comments
Closed

Comments

@abdullahsalem
Copy link
Contributor

I think that when the radius is applied on the button group, it should appear as a wrapper for the whole group not each one!

How can we reproduce this bug?

What did you expect to happen?
button-group-container-radius

What happened instead?
button-group-container-radius-issue

@g12n
Copy link

g12n commented Apr 1, 2016

Perfect! I was just fixing this bug locally!
Thanks @abdullahsalem! Hope the pull request is integrated soon!

@andycochran
Copy link
Contributor

Hmm… I'm not sure if this is a bug.

This makes an assumption that you don't want each button to have its own radius. That definitely makes sense for Split Buttons (w/ the arrow). But if $buttongroup-spacing is set to a larger value, it might look better to have the radius on each button.

I suppose having the radius only on the group does help to associate the buttons with each other as a group. But I'm still not sure if the radius should go on the group or on each button. Maybe what's needed is a $button-group-radius variable. Then you could have it either way.

Honestly, I'm for dropping $button-radius altogether and making it require app-specific customization.

@g12n
Copy link

g12n commented Apr 2, 2016

@andycochran I guess we could add another variable, to make this behavior optional. Though personally I think, the given Example of a split button shows quite clearly, that removing the radius inside the group is the typically expected behavior.

Just for Illustration purposes. This is a split button:
split button
The current result is merely two buttons real close to each other:
two buttons

As for dropping the radius $button-radius completely: I don‘t think this is appropriate. It might be fashionable as it is currently to simplify UI Elements as much as possible. But fashions come and go. Distinguishing a button from a label by adding a hint of radius is quite a common element in the UI Design Toolbox. Even this site here itself is quite a simple reminder of that:

bildschirmfoto 2016-04-02 um 16 52 45

Considering how many developers use this framework already, i would strongly advice against dropping such a central property.

@g12n
Copy link

g12n commented Apr 2, 2016

@abdullahsalem How about adding a variable $buttongroup-inner-radius?

Wrapping a your code additions into a condition @if $buttongroup-inner-radius != $button-radius

This way the code isn't added to the final CSS code, where it is not needed. Keeping the results as lean as possible.

@andycochran
Copy link
Contributor

Even this site here itself is quite a simple reminder of that:
@g12n

^ In that group of GitHub buttons, each button has its own radius.

But again, I totally agree that split buttons should have their radius on the group.
I'm still "split" too; I see benefits to radius on the group and radius on each button.

The reason I'm for dropping $button-radius is: Some of Foundation 6's primary objectives are to reduce the amount of code, make it easy to do things that are hard, and making as few assumptions as possible about aesthetics. And it's extremely easy to round corners with app-specific styles.

Although, I really doubt dropping $button-radius will happen. And I'm not at all opposed to it remaining. There's just a lot of thought that need to go into Buttons, Button Groups, and Menus. I've mentioned this in several other issues: there's a functional overlap between Menus and Button Groups. Foundation could be greatly simplified by merging 'em (e.g. add .button classes to a .menu and get a button group).

The Zurb team probably needs to discuss internally and weigh in on this.

* Sorry for the "split" pun. 😊

@g12n
Copy link

g12n commented Apr 4, 2016

He he!

Well, I guess until be have Feedback from ZURB, we can only work with what is given:

The current implementation of the button-group highly suggest, that the button group is intended as a closed group of buttons, acting as one. Not a group of separate buttons, in a lose relation to each other.

You can see that, if you ramp up the calue of $buttongroup-spacing

bildschirmfoto 2016-04-04 um 09 25 45

The spacing is implemented by changing the size of the buttons border-right and turning it white. Actually this renders the $buttongroup-spacing quite useless, if it exceeds the value of buttons border-thickness.

To cover both use cases, this should be changed to use margin-right for the spacing.

I wouldn't use the .menu element as basis for a button group. The term menu comes with a semantic load, that isn't always applicable for simple buttons. The biggest hurdle against using the menu is it's hard coded reliance on unordered lists. Forcing the use of another semantically loaded element, and taking away freedom for developers to implement it.

Introducing $buttongroup-child-selector greatly improved the usefulness of the tool for us. I agree, that the code could be simplified by merging both elements into a common mixin with identical properties. But this exceeds the scope of this issue.

@g12n
Copy link

g12n commented Apr 4, 2016

I adapted the code of @abdullahsalem to include a $buttongroup-inner-radius variable an use the $button-radius instead of $global-radius.

Check it out here: https://github.com/g12n/foundation-sites/commit/65a4d88d43d8bd7205430f8e4a71d551b749b911

$buttongroup-inner-radius default's to $button-radius. This way the code doesn't change the current behaviour, but gives developers detailed control to decide how button-groups should behave.

Please give me a quick feedback, before i initialize a pull request.

@abdullahsalem
Copy link
Contributor Author

Thank you for your contributing @g12n @andycochran,
I am very busy these days, but will come back soon and read every single comment.
Thanks a lot, everyone.

@abdullahsalem
Copy link
Contributor Author

Hi guys,
I totally agree with @andycochran, we cannot literally say this is a bug, but I strongly believe that the radius should wrap the whole group for the following reasons:

  • The aim of this kind of grouping is to make the user feels that these buttons are related to each other
  • When you wrap the whole group (as I suggested) that will easily help the user to figure out the relationship
  • If the radius go on each button in the group, the user will visually miss that meaning of grouping, then he/she will not be able to differentiate between: the buttons those are related to each other (grouped buttons), and the buttons those come near from each other while no relation between them ( not grouped buttons).

@andycochran
Copy link
Contributor

@abdullahsalem Fair point. But that perspective certainly devalues the function of Button Groups that don't have any radius. Hmm… 

I've been working on a write-up, trying to sort this all out:
http://codepen.io/andycochran/pen/reJRro

(My next step: take an inventory of every function that Button Groups & Menus have/need in order to assess a rewrite.)

@abdullahsalem
Copy link
Contributor Author

Thank you very much @andycochran, you did a great work on that codepen.

I am really not sure if the assumption of multiple lines should be considered in such this component, but I currently don't think so. Therefore, I might choose either option B or C, but I would prefer C if the multiple-lines is accidentally happened.

@kball
Copy link
Contributor

kball commented Apr 25, 2016

Given how much of this is opinion based depending on the details of your app, I kind of wonder if it would be good enough to add a little tutorial on how to implement these either as custom classes or as extensions to the base classes.

One thing that gets lost sometimes is how much Foundation is intended to be just that - a foundation - not the be-all and end-all. If we can give more guidance about how to extend it and build on top of it, some of these issues may become less pressing.

abdullahsalem added a commit to abdullahsalem/foundation-sites that referenced this issue Jul 18, 2016
kball added a commit that referenced this issue Oct 10, 2016
@kball
Copy link
Contributor

kball commented Oct 10, 2016

Fixed in #9031. Will be in 6.3

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

No branches or pull requests

5 participants