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.

Sign up for GitHub

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

Jump to bottom

Fix horizontal .list-group-flush borders. #39513

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Fix horizontal .list-group-flush borders. #39513

wants to merge 6 commits into from

Conversation

WOLFRIEND
Copy link

@WOLFRIEND WOLFRIEND commented Dec 19, 2023 •
edited by julien-deramond
Loading

Description

According to the documentation:

Currently horizontal list groups cannot be combined with flush list groups.

So, when using .list-group-flush and .list-group-horizontal on the same list group, we got redundant borders.
Detailed description and reduced test cases could be found at #39150.

Fixes #39150.

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

#39150

@WOLFRIEND WOLFRIEND requested a review from a team as a code owner December 19, 2023 16:34
@WOLFRIEND WOLFRIEND mentioned this pull request Dec 19, 2023
Open
3 tasks
@WOLFRIEND WOLFRIEND force-pushed the fix-horizontal-list-group-flush branch from 239da0e to 2095f21 Compare December 19, 2023 16:51
louismaximepiton
louismaximepiton requested changes Jan 5, 2024
View reviewed changes
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.

I really like the implementation.
Just a quick fix to do and a proposal for the documentation.

scss/_list-group.scss Outdated Show resolved Hide resolved
site/content/docs/5.3/components/list-group.md Outdated Show resolved Hide resolved
@WOLFRIEND
Copy link
Author

I really like the implementation. Just a quick fix to do and a proposal for the documentation.

Thanks for your suggestions. I've updated the PR, could, you, take a look, please?

mdo
mdo reviewed Jan 9, 2024
View reviewed changes
scss/_list-group.scss Outdated
Comment on lines 176 to 195
// Horizontal flush list items
//
// Remove borders and border-radius to keep horizontal list group items edge-to-edge. Most
// useful within other components (e.g., cards).
@each $breakpoint in map-keys($grid-breakpoints) {
@include media-breakpoint-up($breakpoint) {
$infix: breakpoint-infix($breakpoint, $grid-breakpoints);

.list-group-flush.list-group-horizontal#{$infix} {

> .list-group-item {
border-width: 0 var(--#{$prefix}list-group-border-width) 0 0;

&:last-child {
border-right-width: 0;
}
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Could this all be handled in the same grid map-keys function as .list-group-horizontal?

Copy link
Author

Choose a reason for hiding this comment

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

Could this all be handled in the same grid map-keys function as .list-group-horizontal?

Sure.
I didn't want to mix them on purpose, but if you think it would be preferable, no problem. I've updated the PR. Take a look, please.

@WOLFRIEND WOLFRIEND requested a review from mdo January 9, 2024 14:58
@WOLFRIEND
Copy link
Author

@mdo
Hi, sorry for the trouble.
Please tell me, have you had the opportunity to review the fixes, are there any updates?
Thank you.

julien-deramond
julien-deramond reviewed Jan 29, 2024
View reviewed changes
Copy link
Member

@julien-deramond julien-deramond left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for this PR @WOLFRIEND!
I might have found an edge case with the following code:

<div class="list-group list-group-flush list-group-horizontal">
  <a href="#" class="list-group-item list-group-item-action">
    The current link item
  </a>
  <a href="#" class="list-group-item list-group-item-action">A second link item</a>
  <a href="#" class="list-group-item list-group-item-action">A third link item</a>
  <a href="#" class="list-group-item list-group-item-action">A fourth link item</a>
  <a class="list-group-item list-group-item-action disabled" aria-disabled="true">A disabled link item</a>
</div>

The basic rendering works perfectly, but the top-left border radius should be rounded when the list group item is focused and hovered. Here is an animation showing the issue:

2024-01-29 17 01 57

@WOLFRIEND do you think this could be handled in this PR?

@WOLFRIEND
Copy link
Author

Thanks for this PR @WOLFRIEND! I might have found an edge case with the following code:

<div class="list-group list-group-flush list-group-horizontal">
  <a href="#" class="list-group-item list-group-item-action">
    The current link item
  </a>
  <a href="#" class="list-group-item list-group-item-action">A second link item</a>
  <a href="#" class="list-group-item list-group-item-action">A third link item</a>
  <a href="#" class="list-group-item list-group-item-action">A fourth link item</a>
  <a class="list-group-item list-group-item-action disabled" aria-disabled="true">A disabled link item</a>
</div>

The basic rendering works perfectly, but the top-left border radius should be rounded when the list group item is focused and hovered. Here is an animation showing the issue:

2024-01-29 17 01 57 2024-01-29 17 01 57

@WOLFRIEND do you think this could be handled in this PR?

Thanks. I'll take a look and let you know asap, am I able to fix it.

@WOLFRIEND
Copy link
Author

@julien-deramond

Hi.
I believe I managed to fix this behavior. I've updated the PR, could, you, take a look, please?

horizontal flush borders

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdo mdo Awaiting requested review from mdo

@julien-deramond julien-deramond Awaiting requested review from julien-deramond

@louismaximepiton louismaximepiton Awaiting requested review from louismaximepiton

Assignees
No one assigned
Labels
css v5
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect borders with .list-group-flush.list-group-horizontal
4 participants
@WOLFRIEND @mdo @julien-deramond @louismaximepiton