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

v5: Unify spacing across components #30564

Merged
merged 2 commits into from Apr 30, 2020
Merged

v5: Unify spacing across components #30564

merged 2 commits into from Apr 30, 2020

Conversation

mdo
Copy link
Member

@mdo mdo commented Apr 11, 2020

  • Updates horizontal padding across alerts, cards, dropdowns, list groups, and modals to be our default $spacer of 1rem
  • Reassigns some variables to improve ease of customization
  • Restyles the breadcrumb a smidge to make the background lighter for improved contrast and a little less vertical padding

Fixes #25057 for v5.

@mdo mdo requested a review from a team as a code owner April 11, 2020 21:20
@mdo mdo added this to Inbox in v5 via automation Apr 11, 2020
@MartijnCuppens
Copy link
Member

The spacer changes LGTM.

Restyles the breadcrumb a smidge to make the background lighter for improved contrast and a little less vertical padding

Should be in another PR, or at least in a separate commit. Contrast ratio still doesn't meet 4.5 (4.26), but it's an improvement.

@mdo
Copy link
Member Author

mdo commented Apr 13, 2020

Restyles the breadcrumb a smidge to make the background lighter for improved contrast and a little less vertical padding

Should be in another PR, or at least in a separate commit. Contrast ratio still doesn't meet 4.5 (4.26), but it's an improvement.

Sounds good, I can re-split this shortly. I recall requests for removing the background and padding entirely on the breadcrumb—that might satisfy the color contrast issue then. Thoughts?

@MartijnCuppens
Copy link
Member

Sounds good, I can re-split this shortly. I recall requests for removing the background and padding entirely on the breadcrumb—that might satisfy the color contrast issue then. Thoughts?

I'm ok with that. tbh, I regularly need to remove the background and paddings. The background, padding and border radius can be achieved with utilities anyway

scss/_variables.scss Outdated Show resolved Hide resolved
@ffoodd
Copy link
Member

ffoodd commented Apr 14, 2020

The versions dropdown's current item's background is a bit stuck in .5rem :)

And for breadcrumbs, I strongly agree with @MartijnCuppens (and will probably try to get off every light gray background as of #30548).

Apart from this, I love spacing consistency!

@mdo mdo changed the title v5: Unify spacing across components v5: Unify spacing across components and restyle breadcrumbs Apr 14, 2020
@MartijnCuppens
Copy link
Member

The left padding added by the ol should be removed from the breadcrumbs.

I also realised there is no .75rem padding utility, so maybe we should keep the paddings configurable (0 by default) and null-ify the once that are removed now.

Also the the breadcrumb looks so "naked" now: https://deploy-preview-30564--twbs-bootstrap.netlify.com/docs/4.3/components/breadcrumb/

@MartijnCuppens
Copy link
Member

Maybe we should cover the breadcrumb changes in another PR, @mdo?

@mdo mdo changed the title v5: Unify spacing across components and restyle breadcrumbs v5: Unify spacing across components Apr 30, 2020
@mdo
Copy link
Member Author

mdo commented Apr 30, 2020

Removed breadcrumb restyling, updated the commits, and fixed the docs versions dropdown. Ready for review again!

@mdo mdo requested review from ffoodd and a team April 30, 2020 04:41
Copy link
Member

@MartijnCuppens MartijnCuppens left a comment

Choose a reason for hiding this comment

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

LGTM. Wonderful improvement, @mdo!

v5 automation moved this from Inbox to Approved Apr 30, 2020
@ffoodd
Copy link
Member

ffoodd commented Apr 30, 2020

The only thing to question is having both $spacer / 2 and $spacer * .5: we may normalize this :)

mdo and others added 2 commits April 30, 2020 15:11
Updates alerts, breadcrumbs, cards, dropdowns, list-groups, modals, popovers, and tooltips

Co-Authored-By: Martijn Cuppens <martijn.cuppens@gmail.com>
@MartijnCuppens
Copy link
Member

@ffoodd, good call, fixed that in the latest rebase

@MartijnCuppens MartijnCuppens merged commit 3e73039 into master Apr 30, 2020
v5 automation moved this from Approved to Shipped Apr 30, 2020
@MartijnCuppens MartijnCuppens deleted the v5-unify-spacing branch April 30, 2020 13:17
olsza pushed a commit to olsza/bootstrap that referenced this pull request Oct 3, 2020
Co-Authored-By: Martijn Cuppens <martijn.cuppens@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v5
  
Shipped
Development

Successfully merging this pull request may close these issues.

Inconsistent component default/spacer padding
3 participants