foundation / foundation-sites

The most advanced responsive front-end framework in the world. Quickly create prototypes and production code for sites that work on any kind of device.
https://get.foundation
MIT License
29.66k stars 5.49k forks source link

Vertical rhythm in Foundation 6 #8164

Closed karland closed 6 years ago

karland commented 8 years ago

@gakimball @rafibomb A lot of people love vertical rhythms in their sites, me included. There are external tools to achieve this, but I think it would be nice if F6 would make it a little easier to actually make the appropriate settings in _settings.scss without external dependencies.

The basic idea for achieving vertical rhythm in F6 would be to have all all top/bottom margins and paddings and all line-heights to be a multiple of the $global-lineheight. This is almost already possible now, but here are some necessary changes:

I am happy to make a PR, but before I would like to check with ZURB if this is actually desired.

The old default values for the current line-height-settings and margins or paddings would remain.

erutan commented 8 years ago

+1 for that.

I'd also suggest a consistent naming scheme for line-height variables $global-lineheight and $form-label-line-height bug me. :)

phifa commented 8 years ago

+1

tomByrer commented 8 years ago

I had to remove some of the vertical padding & line-heights in some of of my page sections. A vertical .collapse would be nice also.

andycochran commented 8 years ago

I like the idea of a $header-line-heights map. But forms and buttons are where nailing down baseline grid gets tricky. See #8507.

Here's a recent article that might help alleviate any concerns about pixel-perfect baseline grids: http://zellwk.com/blog/web-typography-broken/

abarnwell commented 8 years ago

+1

brettsmason commented 7 years ago

I'd quite like to pick this up unless anyone else from the original discussion wants to submit a PR?

abarnwell commented 7 years ago

@brettsmason happy to help with this if you want to kick it off.

karland commented 7 years ago

Initially I was checking with ZURB if this is desired. I still think Vertical Rhythm would be a real nice and useful addition. I have not taken the time to work on a complete solution. At the moment I am just using a quick and dirty approach with header-line-heights and some settings.

@mixin myfoundation-vertical-rhythm {

  // Heading line-heights
  @each $size, $headerlhs in $header-lineheights {
    @include breakpoint($size) {
      @each $headerlh, $lheight in $headerlhs {
        #{$headerlh} {
          line-height: $lheight;
        }
      }
    }
  }

}

My settings.scss looks like this:

$header-lineheights: (
  small: (
    'h1': 1.5 * $base-line-height,
    'h2': 1.5 * $base-line-height,
    'h3': 1 * $base-line-height,
    'h4': 1 * $base-line-height,
    'h5': 1 * $base-line-height,
    'h6': 1 * $base-line-height,
  ),
  medium: (
    'h1': 3 * $base-line-height,
    'h2': 2 * $base-line-height,
    'h3': 1.5 * $base-line-height,
    'h4': 1 * $base-line-height,
    'h5': 1 * $base-line-height,
    'h6': 1 * $base-line-height,
  ),
);

// Vertical Rhythm
$global-lineheight: $base-line-height;
$header-lineheight: $base-line-height;
$header-margin-bottom: $base-line-height;
$lead-lineheight: $base-line-height;
$list-lineheight: $base-line-height;
$list-margin-bottom: $base-line-height;
$paragraph-lineheight: $base-line-height;
$paragraph-margin-bottom: $base-line-height;
$subheader-lineheight: $base-line-height;
$subheader-margin-bottom: $base-line-height;
$subheader-margin-top: 0;

Great, if you want to take this up. I am happy to add/support. I also looked at a couple of VR libraries for ideas out of which I particularly liked https://github.com/Pushplaybang/knife/.

brettsmason commented 7 years ago

@karland Great thanks for that and the link, very interesting. I'd love to work through this with you and get this implemented.

My initial thought was to have a separate map, exactly like you have done for line heights, along with maybe margins. Then have the option of assigning this map to the default $header-lineheight variable, as well as $header-margin-bottom. Then in the current code run a check to see if the variable is a map and then output the values in the code if it is, otherwise treat it as a static value as is currently works now.

What do you think? Do you think we also need to account for top margins or assume this is set for all headers globally?

karland commented 7 years ago

@brettsmason Great. At the moment is not a good time for me to work on this, so just push ahead. However, here are some comments:

  1. The first thing is probably to add some test so that we know if it behaves like expected. The test case can later also be used in the kitchen sink.
  2. I personally like Gridlover, so this could be the underlying concept. People who later put settings into F6 could then try these settings in there.
  3. If Gridlover would be the underlying logic this would answer the questions whether to use header-margin-top. I think yes.
  4. You are addressing a principal question with respect to checking for a map and I do not have an answer to this. My hunch would be to start with the settings.scss and introduce all necessary new parameters first, before actually starting with SASS-coding. With this it becomes clear how a solution would look like from the user point of view and if it is clear enough. In general I think the default settings.scss should not come with uncommented parameters.
  5. I guess one question is if vertical rhythm is becoming a core/base feature and therefore the parameter map such as header-lineheights is moving under
//   4. Base Typography

or if all vertical ryhthm parameters go to

//  ??. Vertical Rhythm

In any case your solution with checking for the map sounds good to me in order to not introduce breaking changes.

Having said the before, also the form elements need some re-work, because they are particularly unvertical-rhythm-like. Especially the form-elements strongly rely on a parameter $form-spacing which is heavily used for vertical and(!) horizontal spacing. I have started to experiment with something like this but this is only food for thought and needs finalization first:

$form-label-line-height: $base-line-height;
$form-spacing: 0;
$form-element-height: $base-line-height;
$form-element-margin: 0 0 $base-line-height 0;
$form-element-padding: 0 0 0 $base-line-height/4;
$form-error-margin: -$base-line-height 0 $base-line-height 0;

What do you think?

brettsmason commented 7 years ago

@karland I'll reply in greater depth later (and thanks for a great reply!) but my first idea was something like this: http://codepen.io/brettsmason/pen/WGXKdE

Basically extend the $header-sizing map. Then some checks would need to check if each key has more than one value, if so then the user is using vertical rhythm. Alternatively we could introduce a new variable $vertical-rhythm which is a Boolean and run some stuff based off this.

I'm happy to do all the work but value your input on it to take it in the right direction! I'll have a chat with some of the Zurb guys and see what they think too.

Also I agree forms needs tackling too, but I'd like to concentrate on headings first and do forms after.

brettsmason commented 7 years ago

Just a quick update. I have this: http://codepen.io/brettsmason/pen/rrYkBo as an evolution of the above Codepen. See what you think of this idea. I feel like it would be quite flexible, as you can set a value to null to not include it in the output - eg to inherit the styling from a smaller breakpoint. However the first option is probably easier for the user I guess?

karland commented 7 years ago

@brettsmason Yeah, extending the $header-sizing-map is a much better approach compared to introducing a new $header-lineheights. I like both of your versions, but I would probably prefer your second (evolved) version. The Boolean variable $vertical-rhythm I do not like so much because it may contradict the settings in the $header-sizing-map, i.e. set to false but map suggests VR. Such an error may be difficult to spot and would introduce complexity in the code and documentation.

andycochran commented 7 years ago

The simplest change would be to simply add a responsive $header-line-height map. But it's still going to be complicated to calculate values by which to multiply the $global-lineheight for each header size so that they snap to a vertical grid. Especially since they're unitless.

Here's an idea… 

What about creating an optional line-height-rhythm function that returns a unitless value, based on the element's font-size in pixels, rounded to the line-height-rhythm value?

You could give elements whatever line-height you want, as you currently do. And with line-height-rhythm:false everything works as-is. But with line-height-rhythm:6, every element's line-height would round to the nearest 6px. Or, an element with font-size:1.5625rem (25px) could return values like 1.12 (28px), 1.28 (32px), etc… 

andycochran commented 7 years ago

Heh. Or even simpler, the $header-line-height map could be set in px instead of unitless (as is the $header-sizes map). Easily calculate 'em yourself.

brettsmason commented 7 years ago

@andycochran that's an interesting idea. I guess we need to establish exactly how to approach this. Should we just allow users the freedom to change the line height/margin values or provide a way for them to output values that work with a particular rhythm (a bit like modular scale I guess)?

I don't mind either approach, but it would need to work with other items too (eg forms that @karland mentioned) if we expand what this is applicable to.

andycochran commented 7 years ago

We shouldn't enforce vertical rhythm (or over-engineer it). But we should make it easier to implement.

For headers, it'd be enough to provide a $header-line-height map (with values in px that convert to unitless) and let folks set the line heights to whatever they like, snapping to a rhythm or not. Do the calculations yourself. The defaults could (should?) follow a rhythm.

We should also do this for other elements—e.g. allow the line height of $paragraph-lineheight to be set in px and converted to unitless with the same function. Note: paragraphs are currently on a subpixel line height (25.6px).

brettsmason commented 7 years ago

I think we need more than just line heights, at least for typography. We also need control over top/bottom margins if we are going to do it right.

I still think something in this direction might be better: http://codepen.io/brettsmason/pen/WGXKdE That or expanding the $header-lineheight and $header-margin variables to work as per the $header-sizes map.

The biggest problem I see at the moment is we cant control margins/line heights individually - dont we just need a way to access and change these if required?

brettsmason commented 7 years ago

Having said that I still like your idea of a function and would love to explore how that could work if you have time to discuss further?

andycochran commented 7 years ago

Agreed, we'll need more than line heights. I like the idea of expanding $header-lineheight and $header-margin more than expanding $header-sizes, as we'll be dealing with more than sizes (semantics).

Might it be easier to create a new $header-styles variable map and deprecate the old maps/vars in a future version?

As far as organizing the variables, what you suggest in your codepen is nice and compact. But it might be confusing to have to reference the order of the properties. I'm not sure if either of the following suggestions are better; I just wanted to throw 'em out there to spark discussion.

We could group by styles instead of by element:

$header-styles: (
  small: (
    'font-sizes': (24, 20, 19, 18, 17, 16),
    'line-heights': (28, 24, 20, 20, 20, 16),
    'top-margins': (0, 0, 0, 0, 0, 0),
    'bottom-margins': (24, 20, 20, 16, 16, 16),
  ),
  medium: ( …

^ But that requires you know that the six values are for h1-h6. (We could handle being provided fewer than six values — e.g. h6 is the same size as h5 if you only provide five sizes.)

Or we could be more explicit:

$header-styles: (
  small: (
    'h1': (
      'size': 24,
      'line-height': 28,
      'top-margin': 12,
      'bottom-margin': 24,
    ),
    'h2': (
      'size': 20,
      'line-height': 24,
      'top-margin': 8,
      'bottom-margin': 20,
    ),
    'h3': ( …

^ This would be very long, but it's very clear.

brettsmason commented 7 years ago

@andycochran I agree about $header-sizes not being the ideal variable to use - I just thought it might be easier, but I agree a new map would probably be better and deprecate the other variables (and remove in 6.4 for example). One map would also be easier to output the styles from than multiple.

I'm trying to also think ahead of how we would treat other components with similar functionality once headers are done - do you think ${component}-styles is right, or do you think we should be mentioning rhythm? I guess I'm not sure if everyone will understand rhythm, so styles is probably right.

Out of your examples, I like the simplicity of number 1 (similar to my first example). However number 2 (same as mine here: http://codepen.io/brettsmason/pen/rrYkBo) is a lot more readable. However I think its going to end up being far too long going forward, but happy to discuss that further.

On the one hand your first example would work well if we were to expand to forms in terms of the labels being clear, but I dont know how you would associate the values with anything in that scenario, so we end up with the same issue as my first example.

With comments and good documentation do you think we can adapt one of first examples to work? Like you also mentioned, inheritance is taken care of by not entering a value.

I'm wondering if a vertical rythm function would also be useful. So instead of having to look at modularscale.com etc, we can work it out for them (not sure how needed this is or not).

What about a $vertical-rhythm map, with breakpoint sizes in and a scale in each size? Not sure how exactly this could work yet though.

phifa commented 7 years ago

So sth. like this

$header-styles: (
  small: (
    'h1': (
      'size': 24,
      'line-height': 28,
      'top-margin': 12,
      'bottom-margin': 24,
    ),
    'h2': (
      'size': 20,
      'line-height': 24,
      'top-margin': 8,
      'bottom-margin': 20,
    ),
    'h3': ( …

is basically just plain variables, right? Don't get me wrong, I am just trying to understand, how this is helping with the rythm issue? Also since we are having px/rem and not em's, how does it support the idea of easy and scaled changes in font-size for example? Say for example I have to change h1 from 50 to 70px. Now I also have to change the other values, since they are fix and don't scale proportionaly (as em's do).

Maybe I just don't get vertical rythms and all that, then just ignore my comment :) I am just trying to follow along. Thanks!

brettsmason commented 7 years ago

@phifa Thanks for chipping in, the more the merrier 😄

So the first problem is the user doesn't currently have access to alter the line heights and margins of header elements, hence the need for a new map. That's the underlying issue in my opinion.

I guess this is kind of a vertical rhythm/flexibility change really. Someone may just want to have access to those values, whilst some users may want to go fully into setting a vertical rhythm.

Some users may want to input their own values from http://www.gridlover.net/try or http://www.modularscale.com, but it would be a nice bonus if we could provide a function that could do the maths for them, they just enter a value in the scale, eg rhythm(1). This could be based off the global font size, plus a scale/ratio map.

phifa commented 7 years ago

Saw this today, codepen included: https://twitter.com/trentwalton/status/788810698008637440?refsrc=email&s=11

andycochran commented 7 years ago

@brettsmason Yeah, my first suggestion is definitely more compact. The second one is 38 lines of code for each breakpoint, and you might as well just write raw CSS (which it almost is).

I don't like the idea of a $vertical-rhythm map. We should give folks control over line-heights and margins, allow them to achieve vertical rhythm. But we shouldn't enforce a particular design method (I'm still not convinced pixel-perfect vertical rhythm is crucial).

It would be cool to be able to plug in e.g. rhythm(1) and get a particular scale. But that's not easy to do programmatically, as fonts have various cap heights (which determine where the type is vertically centered in its box). You get into the weeds real quick trying to make sure type's baseline sits exactly on the grid, having to calculate vertical offsets with negative margins… $offset: ($line-height - $cap-height * $font-size) / 2; …which are different for each typeface, not just each font size. And it's going to get even more complicated when we consider elements other than headers and paragraphs.

Maybe we could eventually get there (plugging in rhythms). But there are several things that need to happen first. I say we tackle it like this:

@phifa, maybe the $header-styles map should accept either units or unitless and unitless-line-height() returns them as unitless. It could just pass through unitless values or calculate them if a unit is provided. This way, if your h1's line-height is 1.2, you could change h1 from 50px to 70px without adjusting the other h1 values.

With these two things in place, we're then set up to get more exact with vertical rhythm. Maybe we write a separate pluggable module for vertical rhythm. Something you could @include or not.

brettsmason commented 7 years ago

@andycochran OK sounds like a good plan, I'll start working on this as soon as I can. I agree that pixel perfect vertical rhythm isn't needed - what we need is control and the ability to try and work towards it if the user wants to.

One question - how do you think we should handle the old variables?

karland commented 7 years ago

I 100% support the last suggestions of @andycochran to add $header-styles and the function @function unitless-line-height(), but I guess it should rather be named @function unitless-header-styles().

With respect to length of the targeted code, I believe, there would be a possibility to name arguments, such as:

$header-styles: (
  small: (
    'font-sizes': (h1: 24, h2: 20, h3: 19, h4: 18, h5: 17, h6: 16),
    'line-heights': (h1: 36, h2: 24, h3: 24, h4: 24, h5: 24, h6: 24),
    'top-margins': (h1: 0, h2: 0, h3: 0, h4: 0, h5: 0, h6: 0),
    'bottom-margins': (h1: 24, h2: 24, h3: 24, h4: 24, h5: 0, h6: 0),
  ),
  medium: ( …

This would make the code much shorter and better readable. Also, we could take the current $header-lineheight and $header-margin-bottom values as default values for all unnamed arguements.

@andycochran Other than that I believe we do not need paddings.

@brettsmason With respect to functions such as rhythm(1) and so on, I believe this can at best be a second step and we should not introduce too much complexity in the first step.

With respect to treating $header-sizes, I suspect, it needs to be treated similar to $flex in foundation.scss

  @if not $flex {
    @include foundation-grid;
  }
  @else {
    @include foundation-flex-grid;
  }

This means

  @if $vertical-rhythm {
    @include foundation-vertical-rhythm;
  }

Otherwise, we would lose downward compatibility, because $header-styles would make $header-sizes, $header-lineheight, $header-margin-bottom ineffective. Even though these old variables would not be needed any more after the introduction of $header-styles I am not sure that $header-sizes, $header-lineheight, $header-margin-bottom can be deprecated as early as F6.4?

brettsmason commented 7 years ago

@karland Thanks for feedback, always appreciated 😄 So I Have been experimenting with it a bit. I like your suggestion to label them, but then I'm wondering if we would be better off doing it this way around?

$header-styles: (
  small: (
    'h1': ('size': 24, 'line-height': 28, 'top-margin': 12, 'bottom-margin': 24),
    'h2': ('size': 20, 'line-height': 24, 'top-margin': 8, 'bottom-margin': 20,)
...

We could even go shorter with the keys:

$header-styles: (
  small: (
    'h1': (size: 24, height: 28, top: 12, bottom: 24),
    'h2': (size: 20, height: 24, top: 8, bottom: 20)
...

Using h1, h2 etc as the groupings also makes it far easier to create the Sass to output everything (from a selfish, the person doing it point of view 😄 ).

I like the use of keys as it means we can check if particular keys exist etc.

What do you think?

karland commented 7 years ago

@brettsmason Yes, I think doing it your way round is better. With respect to your two alternatives, I am undecided. In general, I find it best if variables are self-explanatory in which would leave to your suggestion 1 with slight modifications:

$header-styles: (
  small: (
    'h1': ('font-size': 24, 'line-height': 28, 'margin-top': 12, 'margin-bottom': 24),
    'h2': ('font-size': 20, 'line-height': 24, 'margin-top':  8, 'margin-bottom': 20)
...

Are the high-commatas definitely needed for the keys (because of the hyphens)?

However, I also really like the shorter version, since it is clear enough. Maybe these keys make it even clearer?

```SASS
$header-styles: (
  small: (
    'h1': (fsize: 24, lheight: 28, mtop: 12, mbottom: 24),
    'h2': (fsize: 20, lheight: 24, mtop:  8, mbottom: 20)
...
andycochran commented 7 years ago

@karland, @function unitless-line-height() is right. It's only for line heights. And the other header styles will have units. Also, the function could be used for other elements (besides headers) once we get around to making their vertical rhythm more configurable.

phifa commented 7 years ago

So for any of these approaches, would it then be possible to add the unit as well? Like this e.g.:

$header-styles: (
  small: (
    'h1': ('font-size': 24px, 'line-height': 1.5em, 'margin-top': 1em, 'margin-bottom': 1em),

and if not added, it default to px for example.

brettsmason commented 7 years ago

@phifa I'm inclined to agree on making the units flexible. I know we force rems on users for most things, but letting the user use what they want would be cool. @andycochran what do you think?

Also we need to settle on an approach so I can get started working on it for the 6.3 deadline 😄

karland commented 7 years ago

@brettsmason Yes, I agree. After the whole discussion, I would probably start with the last suggestion by @phifa to get going. This is probably also most flexible, as it allows to set the values as multiples of a $base-line-height.

$header-styles: (
  small: (
    'h1': ('font-size': 24px, 'line-height': 1.5*$base-line-height, 'margin-top': $base-line-height, 'margin-bottom': $base-line-height),

If a key is not set, it can default to the current values from _settings.scss, i.e. 'margin-top': 0, 'margin-bottom': $header-margin-bottom, 'line-height': $header-lineheight.

Also, in this approach, the 'line-height' can be unitless anyway. I originally thought, the suggestion of @andycochran was to make all numbers unitless, but that did not seem to be the case?

brettsmason commented 7 years ago

@karland OK great I'll go with that approach. I'm thinking that on everything other than line height we can check if the value has a unit, if it has no unit run it through rem-calc, otherwise we assume we are using our own units.

andycochran commented 7 years ago

As I described above, I think $header-styles should accept line-height values with or without units. And the unitless-line-height() function returns them as unitless. If your line-height is unitless, it's just passed through the function as is. If your line-height is in px, the function calculates and returns a unitless value.

For example, this:

'h1': ('font-size': 24, 'line-height': 30px, 'margin-top': 12, 'margin-bottom': 24) 
'h2': ('font-size': 20, 'line-height': 1.2, 'margin-top': 8, 'margin-bottom': 20) 

…would give you this:

h1 {
  font-size: 1.5rem;
  line-height: 1.25;
  margin-top: 0.75rem;
  margin-bottom: 1.5rem;
} 
h2 {
  font-size: 1.25rem;
  line-height: 1.2;
  margin-top: 0.5rem;
  margin-bottom: 1.25rem;
} 
karland commented 7 years ago

@brettsmason in case you have not already started coding, here are some lines of code that I am experimenting with in my last project. It does not yet use $rem-calc():

// new vars
$base-font-size: 16px !default;
$base-line-height: 24px !default;

// existing foundation vars
$header-lineheight: $base-line-height;
$header-margin-bottom: $base-line-height;

$header-styles: (
  // you can use the long form:
  'small': (
    'h1': ('font-size': 36px, 'line-height': 2*$base-line-height, 'margin-top': 0, 'margin-bottom': $base-line-height),
    'h2': ('font-size': 20px, 'line-height':   $base-line-height, 'margin-top': 0, 'margin-bottom': $base-line-height),
    'h3': ('font-size': 19px, 'line-height':   $base-line-height, 'margin-top': 0, 'margin-bottom': $base-line-height),
    'h4': ('font-size': 17px, 'line-height':   $base-line-height, 'margin-top': 0, 'margin-bottom': $base-line-height),
    'h5': ('font-size': 16px, 'line-height':   $base-line-height, 'margin-top': 0, 'margin-bottom': $base-line-height),
    'h6': ('font-size': 16px, 'line-height':   $base-line-height, 'margin-top': 0, 'margin-bottom': $base-line-height),
  ),
  // you can also use the short form:
  'medium': (
    'h1': ('fs': 48px, 'lh': 3*$base-line-height, 'mt': 0, 'mb': $base-line-height),
    'h2': ('fs': 40px, 'lh': 2*$base-line-height, 'mt': 0, 'mb': $base-line-height),
    'h3': ('fs': 31px, 'lh': 2*$base-line-height, 'mt': 0, 'mb': $base-line-height),
    'h4': ('fs': 25px, 'lh': 2*$base-line-height, 'mt': 0, 'mb': $base-line-height),
    'h5': ('fs': 20px, 'lh':   $base-line-height, 'mt': 0, 'mb': $base-line-height),
    'h6': ('fs': 16px, 'lh':   $base-line-height, 'mt': 0, 'mb': $base-line-height),
  ),
  // you can also leave out any definition using the default values:
  // font-size: 1rem
  // line-height: $header-lineheight
  // margin-top: 0;
  // margin-bottom: $header-margin-bottom
  'large': (
    'h1': ('fs': 48px, 'lh': 3*$base-line-height),
    'h2': ('fs': 30px, 'lh': 2*$base-line-height),
    'h3': ('fs': 24px, 'lh': 2*$base-line-height),
    'h4': ('fs': 20px),
    'h5': ('fs': 16px),
    'h6': ('fs': 16px),
  ),
);

@mixin foundation-vertical-rhythm {  

  @each $size, $headers in $header-styles {
    @include breakpoint($size) {
      @each $header, $header-defs in $headers {
        #{$header} {
          @if map-has-key($header-defs, font-size) {
            font-size: map-get($header-defs, font-size);
          } @else if map-has-key($header-defs, fs) {
            font-size: map-get($header-defs, fs);
          } @else {
            font-size: 1rem;
          }
          @if map-has-key($header-defs, line-height) {
            line-height: map-get($header-defs, line-height);
          } @else if map-has-key($header-defs, lh) {
            line-height: map-get($header-defs, lh);
          } @else {
            line-height: $header-lineheight;
          }
          @if map-has-key($header-defs, margin-top) {
            margin-top: map-get($header-defs, margin-top);
          } @else if map-has-key($header-defs, mt) {
            margin-top: map-get($header-defs, mt);
          } @else {
            margin-top: 0;
          }
          @if map-has-key($header-defs, margin-bottom) {
            margin-bottom: map-get($header-defs, margin-bottom);
          } @else if map-has-key($header-defs, mb) {
            margin-bottom: map-get($header-defs, mb);
          } @else {
            margin-bottom: $header-margin-bottom;
          }
        }
      }
    }
  }

}
brettsmason commented 7 years ago

@karland Thats fantastic and will be very useful, thank you! I've been rebuilding off canvas the last few weeks so I haven't really made any progress with this yet. My aim was to get onto this as soon as off canvas is done (should be done early this week).

I'll properly look through your code and go from there, but thanks again!

brettsmason commented 7 years ago

@karland @andycochran I had a spare hour this afternoon to start looking at this. I thought I'd create the unitless-line-height function first, seeing as @karland 's code above is going to be such a big part of.

So far I have this, what do you think?

/// Converts a pixel, percentage, rem or em value to a unitless value based on a given font size.
///
/// @param {Number} $value - Value to convert to a unitless line height
/// @param {Number} $base - The font size to use to work out the line height - defaults to $global-font-size
///
/// @return {Number} - Unitless number
@function unitless-line-height($value, $base: null) {

  // If no base is defined, defer to the global font size
  @if $base == null {
    $base: $global-font-size;
  }

  // First, lets convert our $base to pixels
  // If the base font size is a %, then multiply it by 16px
  @if unit($base) == '%' {
    $base: ($base / 100%) * 16px;
  }

  // Using rem as base allows correct scaling
  @if unit($base) == 'rem' {
    $base: strip-unit($base) * 16px;
  }

  @if unit($base) == 'em' {
    $base: strip-unit($base) * 16px;
  }

  // Now lets convert our value to pixels too
  @if unit($value) == '%' {
    $value: ($value / 100%) * 16px;
  }

  @if unit($value) == 'rem' {
    $value: strip-unit($value) * 16px;
  }

  @if unit($value) == 'em' {
    $value: strip-unit($value) * 16px;
  }

  @if type-of($value) == 'number' and not unitless($value) {
    @return strip-unit($value) / strip-unit($base);
  }

  @return $value;
}

It allows you to pass in a value to convert, as well as an optional font size to use. I also wonder if we could set the $global-font-size in pixels, and convert it to a percentage when its used for the html elements font size? This would avoid introducing a new variable.

andycochran commented 7 years ago

This is a great start, @brettsmason. Do you think it could be beneficial to abstract this function for purposes other than line height? Not sure what it might be used for. Might there might be a future case where we need to get a number based on a font-size? Maybe call it unitless-value instead of unitless-line-height. Maybe not; just a thought.

brettsmason commented 7 years ago

@andycochran i actually thought about calling it 'unitless-calc' to tie in with rem-calc. What do you think? If you reckon this is good to go I'll start putting everything together. What do you think about the base/global font size issue - introduce a new variable?

andycochran commented 7 years ago

👍 unitless-calc()

andycochran commented 7 years ago

Doesn't this assume that the default font size of every browser is 16px?

brettsmason commented 7 years ago

@andycochran So most of that code was taken from the rem-calc function which does the same. Seeing as thats how its already handled it I assumed this would be OK too. Also wondered if some of the checks should be extracted into their own smaller functions, but its probably OK as is.

karland commented 7 years ago

@brettsmason @andycochran Great stuff.

To test the end result, I have started to think about how to produce a visual test for vertical rhythm to see if all F6 elements eventually align to a vertical rhythm. However, I am not really sure how to deal with this in the current test environment. Currently none of the foundation visual test cases is generated on the fly. So, for the moment I have just come up with a little jsfiddle. Any ideas?

brettsmason commented 7 years ago

@karland Sorry for the delay!

So firstly I have finally started work on this: https://github.com/brettsmason/foundation-sites/tree/vertical-rhythm

So far I have added the new map, and decided to add it in a format that matches the current setup. I also kept pretty much all your code as it was spot on - I just wrapped the values in rem-calc / unitless-calc where appropriate.

What I'm not sure of is how we depreciate the old $header-sizes map, as its no longer needed.

I'd appreciate if you could take a look at the work so far and see what you think 😄

As far as visual tests go, what do you mean by generated dynamically? That fiddle looks like a great starting point though.

karland commented 7 years ago

@brettsmason I have been working on the code, but cannot PR to your brettsmason/foundation-sites/vertical-rhythm-branch as this conflicts with my own fork karland/foundation-sites. @brettsmason @andycochran is it possible to make this a zurb/foundation-sites/vertical-rhythm branch?

brettsmason commented 7 years ago

@karland yes of course give me an hour to get home and I'll do that!

brettsmason commented 7 years ago

@karland I've added the branch and pushed the changes I've made so far: https://github.com/zurb/foundation-sites/tree/vertical-rhythm

karland commented 7 years ago

@brettsmason Excellent. Thank you. I have submitted a PR #9386.

Now for the unitless-calc(), I am not sure, why your unitless-calc() would calculate unitless line-heights. This would mean that the line-height is relative to the font-size. I'd rather suggest to make line-heights in rems. Or am I on the wrong track?

brettsmason commented 7 years ago

@karland Great I'll take a look later today at your PR.

The unitless-calc function was designed to return a unitless line height, as I believe this is the recommended way of giving a line height.

The function takes a base value (eg $base-font-size) plus a value you give it, and it returns the correct line height value based off those values. At the moment its set to $global-font-size as I didnt know if we were going to introduce another variable or not.

Is that not what you were thinking?