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: Fix btn-group-vertical by using same rules as btn-group #40488

Merged
merged 7 commits into from
May 30, 2024

Conversation

tordans
Copy link
Contributor

@tordans tordans commented May 25, 2024

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)+.btn on the right

Bildschirmfoto 2024-05-25 um 07 06 39

Modified CSS:

Bildschirmfoto 2024-05-25 um 07 08 03

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 > .btn.dropdown-toggle-split:first-child to the Reset rounded corners section 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. – Update: I removed this again because the docs state that the split buttons are not supported for vertical button groups.

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing functionality)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Live previews

Related issues

@tordans tordans requested a review from a team as a code owner May 25, 2024 05:13
…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.
Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Screenshot 2024-05-25 at 07 53 00 Screenshot 2024-05-25 at 07 52 55

Copy link
Contributor Author

@tordans tordans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was not supposed to be in here. Will fix soon.

scss/_button-group.scss Outdated Show resolved Hide resolved
@PRERAKAGARWAL28
Copy link

Thanks for catching the issue in the first place and then for submitting a PR @tordans
I have resolved the issue.
https://stackblitz.com/edit/jzsfzg?file=index.html

@tordans
Copy link
Contributor Author

tordans commented May 25, 2024

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.

This is fixed now. The preview looks right to me, now.

Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@julien-deramond julien-deramond requested a review from a team May 27, 2024 19:35
Copy link
Member

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels right to me, couldn't find any regression while playing with it. Thanks for your contribution!

@julien-deramond julien-deramond merged commit 76ed1c6 into twbs:main May 30, 2024
14 checks passed
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.

Wrong border radius first element in btn-group-vertical is a dropdown
4 participants