fix(lib): removed breadcrumb margin bottom#3693
Conversation
github-project-automation
Bot
moved this to In Progress / Draft
in 🟣 [Orange-Boosted-Bootstrap] PRs Board
boosted-bot
moved this from In Progress / Draft
to Need Dev Review
in 🟣 [Orange-Boosted-Bootstrap] PRs Board
✅ Deploy Preview for boosted ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
MaxLardenois
left a comment
MaxLardenois
left a comment
There was a problem hiding this comment.
LGTM, just a question
| // scss-docs-end pagination-css-vars | ||
|
|
||
| display: flex; | ||
| @include list-unstyled(); |
There was a problem hiding this comment.
Is there a reason to move this?
There was a problem hiding this comment.
Yes, if I were to leave it as initially set, the bottom margin would be set to 0 instead of --#{$prefix}pagination-margin-y) (line 39)
There was a problem hiding this comment.
Ah yes good point, wdyt about that @louismaximepiton?
People might not get that list-unstyled adds a margin-bottom. We might have to document this at least in migration?
There was a problem hiding this comment.
I totally agree on this, we should probably add an info or warning level on the migration guide. But we don't have the mixins documented right now, so if people use them, it's probably that they know what they do. Probably an info level would be enough
There was a problem hiding this comment.
Info added to sass section of Migration from boosted
There was a problem hiding this comment.
Thanks, I think we should add it in migration.mdx too in the future 1.5 version
boosted-bot
moved this from Need Dev Review
to Need Lead Dev Review
in 🟣 [Orange-Boosted-Bootstrap] PRs Board
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #3611 by removing default bottom spacing from lists used by components like Breadcrumb, by adding margin-bottom: 0 to the shared list-unstyled mixin and adjusting affected component styles/output accordingly.
Changes:
- Updated
@mixin list-unstyledto resetmargin-bottomto0. - Removed now-redundant explicit
margin-bottom: 0in a few consumers and reordered@include list-unstyled()in pagination to avoid overriding pagination margins. - Updated SCSS snapshot outputs to reflect the new generated CSS.
Reviewed changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| site/src/scss/_toc.scss | Removes redundant margin-bottom: 0 now handled by list-unstyled. |
| scss/mixins/_lists.scss | Adds margin-bottom: 0 to list-unstyled mixin (core behavior change). |
| scss/_pagination.scss | Moves @include list-unstyled() earlier so pagination’s margin: isn’t overridden. |
| scss/_navbar.scss | Removes redundant margin-bottom: 0 now handled by list-unstyled. |
| scss/tests/snapshot-tests/snapshots/ouds-web.css | Updates expected compiled CSS snapshot for new list styles/order. |
| scss/tests/snapshot-tests/snapshots/ouds-web-bootstrap.css | Updates expected compiled CSS snapshot for new list styles/order. |
Files not reviewed (2)
- scss/tests/snapshot-tests/snapshots/ouds-web-bootstrap.css: Generated file
- scss/tests/snapshot-tests/snapshots/ouds-web.css: Generated file
| // scss-docs-end pagination-css-vars | ||
|
|
||
| display: flex; | ||
| @include list-unstyled(); |
There was a problem hiding this comment.
I totally agree on this, we should probably add an info or warning level on the migration guide. But we don't have the mixins documented right now, so if people use them, it's probably that they know what they do. Probably an info level would be enough
2124571 to
454f925
Compare
Types of change
Related issues
Closes #3611
Description
Added margin-bottom set to 0 to list-unstyled mixin, and updated components that use it accordingly.
Checklists
Checklist (for Core Team only)
Progression (for Core Team only)
mainfollowing conventional commitLive previews