Vertical Rhythm is Off

[nsteffan]

I took a look at the issue here: https://github.com/chriseppstein/compass/issues/806

I believe the original issue submitter is correct.

When I do: h1 { @include adjust-font-size-to($base-font-size * 3); }

I get: h1 { font-size: 3em; line-height: 3.375em; }

What that is actually saying is the font-size is 3 times that of the base font size (16px). By specifying a line-height of 3.375em, you are actually creating a line-height of (16px x 3 x 3.375) 162 px. That is not right at all.

When I get a chance, I will see about creating a patch. Thanks.

Sources: http://www.w3schools.com/cssref/css_units.asp http://www.w3schools.com/cssref/pr_dim_line-height.asp

[nsteffan]

One thing I determined was the configuration and import of the vertical rhythm file matter:

This properly sets the vertical rhythm up.

@import "compass/reset";

// Font Sizes $relative-font-sizing : false; $base-font-size : 16px; $base-line-height : 18px; $round-to-nearest-half-line : true;

@import "vertical_rhythm";

// establish your vertical baseline grid. @include establish-baseline;

h1 { @include adjust-font-size-to(48px); }

p { &:first-child { @include leader;
} @include trailer; }

This doesn't:

@import "compass/reset"; @import "vertical_rhythm"; // Local Copy of Vertical Rhythm

// Font Sizes $relative-font-sizing : false; $base-font-size : 16px; $base-line-height : 18px; $round-to-nearest-half-line : true;

// establish your vertical baseline grid. @include establish-baseline;

h1 { @include adjust-font-size-to(48px); }

p { &:first-child { @include leader;
} @include trailer; }

[chriseppstein]

@snugug @johnalbin Thoughts?

[Snugug]

@chriseppstein Yah, he's right about the line height in ems thing, what can I do to help? Would we prefer to do the em calculation and spit out what we want from rems too (which is what's actually being calculated w/the line height) or just ems? Personally, I don't think we need to spit out the rems if we're calculating the real ems.

Also, I haven't looked at the code, but if Vertical Rhythm is making calculations w/o being wrapped with a mixin, then it's going to use the !default instead of the variables the user sets; it's this same reason why I've been including a "kickstart" mixin for all of my extensions that need to do initialization calculations, allowing the user to import wherever they'd like and simply call the "kickstarter" when they're ready to start using.

[nsteffan]

I think doing something similar to a bootstrap or kickstart mixin may be the proper way to approach this, or possibly include the functionality inside the establish-baseline mixin as anyone using the vertical rhythm plugin will probably be making this call. Another option would be to create the mixin outside of the establish-baseline mixin, but then have the establish-baseline mixin call the bootstrap/kickstart mixin.

[mdeltito]

I'm seeing a similar result: https://gist.github.com/3249043

Note that I'm not setting relative-font-sizing to false

I expected the output to properly adjust both the font-size and line-height down to the proper value to maintain the rhythm. Instead like the example from nsteffan above I'm seeing the line-height adjusted to match the baseline px value.

I'm not well-versed in working with vertical rhythm in general, so it's very possible I'm doing something wrong. If this is in fact an issue (or just the way it works) is there another method I should be used to properly adjust line-height along with font-size whilst maintaining the rhythm?

[mirisuzanne]

I can't recreate the first part of this. When I input:

h1 { @include adjust-font-size-to($base-font-size * 3); }

I get this output:

h1 {
  font-size: 3em;
  line-height: 1.33333em;
}

That's correct.

[mirisuzanne]

The Compass tests are also providing correct results:

// in
$base-font-size: 14px;
$base-line-height: 16px;
.small { @include adjust-font-size-to(12px,1); }

// out
.small {
  font-size: 0.857em;
  line-height: 1.333em; }

[nsteffan]

Ericam, my issue stems from when $relative-font-sizing : false is set. Please look at my second comment for a reproducible issue.

[mirisuzanne]

I see. It's all one issue, easily solved by a kickstart:

@mixin kickstart {
  $font-unit: if($relative-font-sizing, 1em, $base-font-size);
  $base-rhythm-unit: $base-line-height / $base-font-size * $font-unit;
  $base-leader: ($base-line-height - $base-font-size) * $font-unit / $base-font-size;
  $base-half-leader: $base-leader / 2;
}

Which can be called by establish-baseline, as mentioned.

However, much of this is changing in #896 anyway. I wonder if we can completely remove the need for calculations outside mixins/functions? That always seems like the safest route to me.

[ry5n]

So it seems the fundamental issue here is that you need to set up your variables before importing the vertical rhythm partial for things to be set up properly. With my proposed changes in #896, I believe this is still the case, but as Eric points out this should be fixable by doing setup in the establish-baseline mixin.

There are a few other fixes I need to make based on existing feedback, so I will look to fix this issue while I'm at it. I've been swamped with extra work, so look for an update to the pull request the first week of September.

@mdeltito I think you might be confused. Basically, we're setting up a grid on which to align text throughout a layout – it's kind of like a grid system for layout but turned on its side. In your example, keeping to the rhythm means maintaining a computed line-height of 24px (or multiples thereof) no matter the size of the type itself. So, it's working! :)

[mdeltito]

@ry5n understood, I knew I was missing something. I thought that the vertical rhythm was meant to keep a consistent ratio between the font-size and the line-height, instead of maintaining the same vertical "gutter" size. Thanks for clarifying :)

[ry5n]

FYI The first updates the the vertical rhythm module have been merged in to master. The setup mixin described above is not included. It is a low priority, since setting configurable variables before importing frameworks is part of Compass' design. I have plans to consider some follow-ups, but it may take time, and my first priority is updating the vertical rhythm docs.