Currently, most of the components/variables mixins don't follow vertical rhythm:
http://jsbin.com/tuyusogoga/edit?html,css,output
Most of the vertical spacings are based on the base font em size, but they should be based on the base line height.
This should be fixed by using typographic mixins, for example:
@function rhythm ($lines: 1, $line-height: $line-height) {
@return ($lines * $line-height) * 1rem;
}
@mixin font-size ($ratio, $lines: 0) {
@if (not unitless($ratio)) {
@if (unit($ratio) == 'rem') {
$ratio: $ratio / ($ratio * 0 + 1);
} @else {
@error 'font-size ratio must be unitless or in rems';
}
}
font-size: $ratio * 1rem;
line-height: if($lines > 0, rhythm($lines), rhythm(ceil($ratio / $line-height)));
}
See http://jsbin.com/buxelurori/1/edit?html,css,output
Moreover, there are still a handfull of places where the spacing is hardcoded, or, if it's using a variable, it is multiplied by an arbitrary number (or a number that makes sense only with the default 1.5 line-height).
For example, the spacing utility classes use the $spacer-y variable multiplied by 1.5 or 3, which breaks if the base $line-height is set to anything other than 1.5.
// scss/_variables.scss
$spacer: 1rem !default;
$spacer-x: $spacer !default;
$spacer-y: $spacer !default;
$spacers: (
0: (x: 0, y: 0),
1: (x: $spacer-x, y: $spacer-y),
2: (x: ($spacer-x * 1.5), y: ($spacer-y * 1.5)),
3: (x: ($spacer-x * 3), y: ($spacer-y * 3))
) !default;
Instead it should be
$spacer: $line-height * 1rem !default;
$spacer-x: $spacer !default;
$spacer-y: $spacer !default;
$spacers: (
0: (x: 0, y: 0),
1: (x: $spacer-x, y: $spacer-y),
2: (x: ($spacer-x * 2), y: ($spacer-y * 2)),
3: (x: ($spacer-x * 3), y: ($spacer-y * 3))
) !default;
A quick grep of the scss codebase:
Hardcoded margins:
- scss/_forms.scss
- 184: margin-bottom: ($spacer * .75);
- 202: margin-top: .25rem;
- 203: // margin-top: 4px \9;
- 204: margin-left: -1.25rem;
- 210: margin-top: -.25rem;
- 226: margin-left: .75rem;
- scss/_dropdown.scss
- 13: margin-right: .25rem;
- 14: margin-left: .25rem;
- 191: margin-bottom: 2px;
- scss/_images.scss
- 46: margin-bottom: ($spacer-y / 2);
- scss/mixins/_navbar-align.scss
- 7:// margin-top: (($navbar-height - $element-height) / 2);
- 8:// margin-bottom: (($navbar-height - $element-height) / 2);
- scss/_carousel.scss
- 130: margin-top: -10px;
- 136: margin-left: -10px;
- 140: margin-right: -10px;
- 168: margin-left: -30%;
- 230: margin-top: -15px;
- 234: margin-left: -15px;
- 237: margin-right: -15px;
- scss/_list-group.scss
- 135: margin-bottom: 5px;
- scss/_code.scss
- 39: margin-bottom: 1rem;
[Edit: Alerts are covered by #18884 – @cvrebert]
scss/_alert.scss
* 17: margin-top: 5px;
[Edit: Covered by #18820 – @cvrebert]
* scss/_media.scss
* 17: margin-top: 15px;
* 78: margin-bottom: 5px;
- scss/_custom-forms.scss
- 19: margin-left: 1rem;
- 127: margin-bottom: .25rem;
Hardcoded paddings:
- scss/_forms.scss
- 187: padding-left : 1.25rem;
- 218: padding-left : 1.25rem;
- 265: padding-right : ($input-padding-x * 3);
- 321: // padding-right : ($input-height * 1.25);
- scss/_dropdown.scss
- 46: padding: 5px 0;
- 70: padding: 3px 20px;
- 150: padding: 3px 20px;
- scss/_custom-forms.scss
- 15: padding-left: 1.5rem;
- 148: padding: .375rem 1.75rem .375rem .75rem;
- 149: padding-right: .75rem \9;
- 174: padding-top: 3px;
- 175: padding-bottom: 3px;
- 216: padding: .5rem 1rem;
- 237: padding: .5rem 1rem;
- scss/_type.scss
- 79: padding: .2em;
- 100: margin-right: $list-inline-padding;
- 117: padding: ($spacer / 2) $spacer;
- 136: padding-right: $spacer;
- 137: padding-left: 0;
- scss/_card.scss
- 163: padding: 1.25rem;
- scss/_navbar.scss
- 72: padding-top: .25rem;
- 73: padding-bottom: .25rem;
- 90: padding-top: .425rem;
- 91: padding-bottom: .425rem;
- 108: padding: .5rem .75rem;
- 151: padding-top: .425rem;
- 152: padding-bottom: .425rem;
- scss/_popover.scss
- 8: padding: 1px;
[Edit: The following 2 lines are covered by #18821 – @cvrebert]
106: padding: 8px 14px;
116: padding: 9px 14px;
- scss/_carousel.scss
- 207: padding-top: 20px;
- 208: padding-bottom: 20px;
- 245: padding-bottom: 30px;
- scss/_list-group.scss
- 19: padding: .75rem 1.25rem;
- scss/_code.scss
- 11: padding: .2rem .4rem;
- 20: padding: .2rem .4rem;
[Edit: Alerts are covered by #18884 – @cvrebert]
* scss/_alert.scss
* 38: padding-right: ($alert-padding + 20px);
[Edit: scss/_variables.scss used be to here, but calling that "hardcoded" would be absurd. – @cvrebert]
[Edit: Covered by #18820 – @cvrebert]
* scss/_media.scss
* 64: padding-left: 10px;
* 68: padding-right: 10px;
- scss/_labels.scss
- 8: padding: .25em .4em;
- 44: padding-right: .6em;
- 45: padding-left: .6em;
ju1ius
All 22 comments
Hi @ju1ius!
You appear to have posted a live example (http://jsbin.com/tuyusogoga/edit), which is always a good first step. However, according to Bootlint, your example has some Bootstrap usage errors, which might potentially be causing your issue:
- W002:
<head>is missing X-UA-Compatible<meta>tag that disables old IE compatibility modes
You'll need to fix these errors and post a revised example before we can proceed further.
Thanks!
(_Please note that this is a fully automated comment._)
twbs-lmvtfy
on 15 Jan 2016
Hi @ju1ius!
You appear to have posted a live example (http://jsbin.com/buxelurori/1/edit), which is always a good first step. However, according to Bootlint, your example has some Bootstrap usage errors, which might potentially be causing your issue:
- W002:
<head>is missing X-UA-Compatible<meta>tag that disables old IE compatibility modes
You'll need to fix these errors and post a revised example before we can proceed further.
Thanks!
(_Please note that this is a fully automated comment._)
twbs-lmvtfy
on 15 Jan 2016
X-Ref: #11601
cvrebert
on 15 Jan 2016
@cvrebert I saw that issue but there was none currently open for v4...
ju1ius
on 15 Jan 2016
Yep, I'm just linking the two related issues together.
cvrebert
on 15 Jan 2016
We have pull requests to un-hardcode several of these. I've struck the relevant portions of your post and added links to the relevant open PRs.
cvrebert
on 15 Jan 2016
@cvrebert Note that these PRs add variables for the relevant components, but the variable values are still hardcoded. They should by default be computed from the $line-height, while taking care that the sum of (line-height [or height for fixed-height components] + margin-top + border-top-width + padding-top + padding-bottom + border-bottom-width + margin-bottom) is always an integer multiple of the base line-height.
The issue is less serious for components like modals or popovers, since they're usually positioned out of the document flow.
ju1ius
on 15 Jan 2016
[Edit: scss/_variables.scss used be to here, but calling that "hardcoded" would be absurd. – @cvrebert]
I disagree...
See my above comment. Default margin & paddings should be computed from the base line-height.
ju1ius
on 15 Jan 2016
Huh.
CC: @mdo
cvrebert
on 15 Jan 2016
Here's a proof of concept: https://github.com/ju1ius/bootstrap/commit/a01a5f4b52ed55f3d382728661fd3d04e4298232, focusing only on basic typography & spacings.
ju1ius
on 15 Jan 2016
@cvrebert Note that this prototype works only if we support a single line-height.
For true responsive typography & vertical rhythm, we must introduce typographic breakpoints:
// could also default to $grid-breakpoints
$typographic-breakpoints: (xs: 0, md: 768px, xl: 1200px);
$font-sizes: (
xs: (14px, 1.428), // 14px/20px
md: (16px, 1.5), // 16px/24px
xl: (18px, 1.333) // 18px/24px
);
@function font-size-at($breakpoint) {
@return nth(map-get($font-sizes, $breakpoint), 1);
}
@function line-height-at($breakpoint) {
@return nth(map-get($font-sizes, $breakpoint), 2);
}
@each $breakpoint in map-keys($typographic-breakpoints) {
@include media-breakpoint-up($breakpoint, $typographic-breakpoints) {
html {
font-size: font-size-at($breakpoint);
line-height: line-height-at($breakpoint);
}
body {
font-size: 1rem;
line-height: line-height-at($breakpoint) * 1rem;
}
}
}
This adds a layer of complexity but has the advantage of being able to specify fixed heights in pixels without breaking vertical rhythm (since we can now compute rems from the font-size and adjust margins/paddings as needed).
ju1ius
on 16 Jan 2016
Including responsive typography in Bootstrap itself was rejected the last time it was brought up; see https://github.com/twbs/bootstrap/issues/17095#issuecomment-133769949
cvrebert
on 16 Jan 2016
Redeclaring the font-size for everyone doesn't make sense. I'm in favor of documenting how to do this, but we won't be adding it ourselves to Bootstrap's default setup.
The point of having sane & simple default is very obvious.
However, with the tools in place it is easy to switch to a simpler setup for the main dist.
Just declare:
$typographic-breakpoints: (xs: 0);
$font-sizes: (xs: (16px, 1.5));
And let the functions & mixins do the rest without bloating the default bundle.
People wanting responsive type can still configure the framework in sass.
Just as it is possible right now with the spacing utilities:
$spacers: (
0: (
x: 0,
y: 0
),
1: (
x: $spacer-x,
y: $spacer-y
),
2: (
x: ($spacer-x * 2),
y: ($spacer-y * 2)
),
3: (
x: ($spacer-x * 3),
y: ($spacer-y * 3)
),
half: (
x: $spacer-x / 2,
y: $spacer-y / 2
),
two-third: (
x: $spacer-x * 2/3,
y: $spacer-y * 2/3
),
golden-ratio: (
x: $spacer-x * 1.618,
y: $spacer-y * 1.618
),
zomg-ponies: (
x: $spacer-x * 42,
y: $spacer-y * 42
)
); // 112 spacing classes !
But anyway this issue is more about vertical rhythm, which would be nice to have even with only one configurable font-size/line-height combination.
ju1ius
on 16 Jan 2016
That sounds like an unnecessary layer of abstraction for folks that just want simple typography.
Most of the vertical spacings are based on the base font em size, but they should be based on the base line height.
I can only really see that being super beneficial with long form bodies of text. The differences we'd see in headings likely wouldn't amount to much (e.g., if the <h1> line height was 1.1).
All told, I'm not seeing a strong enough argument to rewrite all our type styles this far into v4. Aside from that mathematical inaccuracy and impact I see on larger bodies of text, why else would we want to do this?
mdo
on 16 Jan 2016
(Fwiw, I'm in favor of more variables as @cvrebert has linked to and what not, but I'm not sold on these mixins.)
mdo
on 16 Jan 2016
Hi !
That sounds like an unnecessary layer of abstraction
That is a layer of abstraction indeed. Not that big however - imo. I think that «vertical spacing is based on line-height (so $line-height * 1rem)» is not a huge mental shift from «all spacing is based on the font height».
Implementation-wise, it would not be that complex (as long as you stick with one configurable base line-height). In the abovementioned prototype, it's actually one function and one mixin. The rest is mostly replacing magic numbers by fuction/mixin calls, some calc() expressions here and there to account for the border-widths, add a few more spacing utility classes for flexibility.
Of course, you can never expect everything to stick to the baseline grid (especially dynamic height content like images, videos), and there are times when you do need more freedom, so the point here is not to be pedantically strict about VR, but to come up with something simple, flexible, which would enhance the framework's default.
While it could be not-so-big-a-change, I understand it needs time & reflexion and that it might be coming a little too late for the v4 release party.
Why I think having VR by default would be easier:
- If you do not care about it, it's easy to opt-out. You just continue not caring, tweaking the variables as you see fit, and have a happy day. (if you don't care about it you probably won't see a difference in the first place anyway).
- If you do care you don't have to override half of the framework, including Reboot, you don't end up with a 300k stylesheet, and you have a happy day.
- One of the most popular css frameworks has strong default typography, and the world becomes a better place.
Just my 2¢, thanks for your mind and time !
ju1ius
on 17 Jan 2016
I think this would be a good candidate to keep in mind for v5.
thatguyyoulove
on 19 Jan 2016
This is actually the one reason I'm still thinking about another frameworks whenever I use Bootstrap. Just wanted to throw that out there.
Kingdutch
on 3 Feb 2016
And maybe use the negative margin on .row also vertically. So we can have divs in cols with the same space all around.
Martinomagnifico
on 4 Feb 2016
Punting on this for v4. Thanks though!
mdo
on 9 Feb 2016
I'm a bit confused by the closed state of this issue. Does v4 respect vertical rhythm?
gpakosz
on 29 Nov 2016
This issue was about adding a specific proposed set of mixins related to vertical rhythm.
The proposal was rejected, although it might be open to reconsideration after v4 has a stable release.
cvrebert
on 29 Nov 2016
Related issues
devfrey
·
3Comments
tiendq
·
3Comments
steve-32a
·
3Comments
ziyi2
·
3Comments
iklementiev
·
3Comments
Most helpful comment
This is actually the one reason I'm still thinking about another frameworks whenever I use Bootstrap. Just wanted to throw that out there.