button-group: Fix btn-group-vertical by using same rules as btn-group
#40488
Conversation
…first child ad8a43b removed a section which had an additional dropend class. This preserves this section and only add the new test case a the top.
There was a problem hiding this comment.
Thanks for catching the issue in the first place and then for submitting a PR @tordans
I haven't checked out the code yet, but there's an issue with the rendering in the deployed version at https://deploy-preview-40488--twbs-bootstrap.netlify.app/docs/5.3/components/button-group/#vertical-variation; the top-right corner from the 2nd item in the vertical variation is rounded, and the left-bottom corner of the last item should be rounded.
tordans
left a comment
tordans
left a comment
There was a problem hiding this comment.
This change was not supposed to be in here. Will fix soon.
|
Thanks for catching the issue in the first place and then for submitting a PR @tordans |
This is fixed now. The preview looks right to me, now. |
We only need this once.
julien-deramond
left a comment
julien-deramond
left a comment
There was a problem hiding this comment.
Thanks again for the PR, @tordans. It's really appreciated.
I've tested the rendering with several combinations of buttons and dropdowns, and it seems to work perfectly now. Even with some outlined versions, the rendering is acceptable. The following mention in the documentation is still valid, even if it shouldn't be too hard to adapt in the future:
Split button dropdowns are not supported here.
The comment follows what exists already for the simple .btn-group, and the new code is scoped for .btn-group-vertical only. This makes it very isolated, without extra impacts on the rest of the code.
Regarding the documentation, having the dropdown in the first position will make it easier for us to maintain in the future and to spot potential regressions. We do lose a little bit of the logic behind the rendering of the set of dropdowns' variants (with two buttons in the middle), but I think it's OK. Otherwise, we could even drop the buttons. IMO, let's leave it as is. If other maintainers have stronger opinions, feel free to directly modify the code.
IMO, it's safe to merge. @twbs/css-review, I'll leave it open for now until you can double-check if you want.
louismaximepiton
left a comment
louismaximepiton
left a comment
There was a problem hiding this comment.
Feels right to me, couldn't find any regression while playing with it. Thanks for your contribution!
4 participants
Description
The CSS conditions for vertical button groups was different from the regular button group.
When using the same conditions for the regular groups for the vertical group, the bug is fixed.
Current CSS:
See the
.btn-group>:not(.btn-check)+.btnon the rightModified CSS:
I am assuming the css for the regular button group was improved at some point but not applied to the vertical button group at this point.
The second change is to add– Update: I removed this again because the docs state that the split buttons are not supported for vertical button groups.> .btn.dropdown-toggle-split:first-childto theReset rounded cornerssection which is also copied down from the regular button group. I am not sure what that does but I think the same logic applies to assume that both button groups should behave the same.I have modified the docs page of the second example on https://getbootstrap.com/docs/5.2/components/button-group/#vertical-variation to have a button group in the first position.
Type of changes
New feature (non-breaking change which adds functionality)Refactoring (non-breaking change)Breaking change (fix or feature that would change existing functionality)Checklist
npm run lint)I have updated the documentation accordinglyI have added tests to cover my changesLive previews
Related issues
btn-group-verticalis a dropdown #40487