-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Fix $buttongroup-spacing #8616
Fix $buttongroup-spacing #8616
Conversation
I love how much cleaner this ends up being. @brettsmason @JeremyEnglert can one of you try this out and verify it works as described? |
@andycochran I've had a look at this. The flex grid version works fine from what I can see. However on the expanded buttons the last item is dropping down a line (this seems to be the case on the current docs too). |
@brettsmason I can't seem to replicate the last item wrapping to the next line. Is this happening in a specific browser? |
@andycochran It was in the latest version of Chrome on Windows 10. I've left the office for the day now so I'll confirm in the morning (I setup some visual tests to confirm everything was OK). |
@andycochran Sorry it took a while to get back to you. I've just pulled down this again and checked at work, the last item is still dropping down. This happens in Chrome and in Edge. See below (first is this PR, 2nd is live docs). I haven't had time to see why this is yet, but I'll take another look when I'm home. |
@brettsmason Boo. I bet this is a sub-pixel rounding bug. The |
I'll try and have a look over your code tonight if I get time and see if I can think of a solution. |
@brettsmason Maybe negative margin on the last child? :last-child {
margin-#{$global-right}: $buttongroup-spacing*-1;
} |
@andycochran I think I've found the issue finally. Line 83 has this: Take a look, I think thats whats causing the issue anyway... |
@brettsmason We actually don't need that @for $i from 2 through $buttongroup-expand-max {
&:first-child:nth-last-child(#{$i}) {
&, &:first-child:nth-last-child(#{$i}) ~ #{$selector} {
display: inline-block;
width: calc(#{percentage(1 / $i)} - #{$buttongroup-spacing});
margin-#{$global-right}: $buttongroup-spacing;
&:last-child {
margin-#{$global-right}: $buttongroup-spacing * -$buttongroup-expand-max;
}
}
}
}
|
@andycochran I've tested that and it works perfectly. I tried up to 6 buttons with lots of spacing configurations in different browsers and all worked as it should. If you can update the PR and I'll merge it. Good job! |
@brettsmason Should be good to go. |
@andycochran great - I've pulled it down to check and all works perfectly. |
border-bottom: $buttongroup-spacing solid $body-background; | ||
} | ||
&:last-child { | ||
margin-bottom: 0; |
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.
I think this is the reason for #8780
This PR fixes some issues with button margins.
.stacked
was using borders for$buttongroup-spacing
)$buttongroup-spacing