Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Vertical rhythm improvements #9419

Merged
merged 11 commits into from
Dec 1, 2016
Merged

Vertical rhythm improvements #9419

merged 11 commits into from
Dec 1, 2016

Conversation

brettsmason
Copy link
Contributor

This PR is hopefully the start of improving control of vertical rhythm throughout the framework.

Features:

  • unitless-calc function for outputting a unitless value - ideal for line heights
  • Introduce new map $header-styles which replaces $header-sizes (including backwards compatibility via build_from_header-sizes function)
  • full control over font size, line height, margin top/bottom per header, per breakpoint
  • Visual test to see the current state of Foundations vertical rhythm

Big thanks to @karland for doing most of the work here!

@kball @andycochran @ncoden I'd love to get this into 6.3 so if anyone can have a look that would be great 😄

brettsmason and others added 9 commits November 16, 2016 20:10
Basically cut out else branches and re-introduce default values for
margin-top and margin-bottom. Update of docs.
Put `font-size` and `line-height` first.
@karland
Copy link

karland commented Nov 30, 2016

@brettsmason This PR still contains my original thought to include settings.scss which I used for testing. However, in the meantime, I'd rather think, it should be like this PR #9396.

@brettsmason
Copy link
Contributor Author

@karland Great thanks for spotting that. Could you check that I have reverted that correctly? I'll leave the settings file change up to your other PR.

@karland
Copy link

karland commented Nov 30, 2016

@brettsmason Looks good.

@kball
Copy link
Contributor

kball commented Nov 30, 2016

So one thing that's striking me as a bit odd is that we mention that $header-styles is getting built from $header-sizes, but removed $header-sizes from _settings.scss... I think octophant is going to just put it back when we do the build... If we're deprecating $header-sizes in favor of $header-styles I'd also like us to put some form of warning in around that...

Can we do something like check for if someone is using $header-sizes or $header-styles and in the case of the former print a deprecation warning?

@brettsmason
Copy link
Contributor Author

@karland are you able to put something together based on what @kball said (or advise on a change)?
I'm struggling for time today to look at it.

@karland
Copy link

karland commented Nov 30, 2016

@kball @brettsmason Hmm, good spotting. I was not aware that _settings.scss is generated up until yesterday night. I ran gulp deploy and now I have this (what I don't want) in _settings.scss:

$header-sizes: (
  small: (
    'h1': 24,
    'h2': 20,
    'h3': 19,
    'h4': 18,
    'h5': 17,
    'h6': 16,
  ),
  medium: (
    'h1': 48,
    'h2': 40,
    'h3': 31,
    'h4': 25,
    'h5': 20,
    'h6': 16,
  ),
);
$header-styles: build_from_header-sizes($header-sizes);

I have to think about it.

The depreciation warning is easier to do. @kball what is the standard procedure for depreciation warnings in F6. Do you want something like @warn?

@brettsmason
Copy link
Contributor Author

@karland Yeah I believe a @warn will be fine.
Let me know if you need any help in working out what to do re settings.

@kball
Copy link
Contributor

kball commented Nov 30, 2016

@karland yep, my thinking was if they set $header-sizes instead of $header-styles they should get a @warn deprecation notice

@karland
Copy link

karland commented Nov 30, 2016

@brettsmason @kball I think I got it. This wasn't difficult. Just please follow my reasoning to double-check.

If I understand correctly Octophant is only considering variables/functions if they have a preceding comment with 3 slashes. So I added a !default map for $header-styles with /// and took out $header-sizes. I also took away one slash from $header-styles: build_from_header-sizes($header-sizes) !default; and wrapped it in an @if variable-exists(header-sizes). This yields in _settings.scss (I tested this!):

$header-styles: (
  small: (
    'h1': ('font-size': 24),
    'h2': ('font-size': 20),
    'h3': ('font-size': 19),
    'h4': ('font-size': 18),
    'h5': ('font-size': 17),
    'h6': ('font-size': 16),
  ),
  medium: (
    'h1': ('font-size': 48),
    'h2': ('font-size': 40),
    'h3': ('font-size': 31),
    'h4': ('font-size': 25),
    'h5': ('font-size': 20),
    'h6': ('font-size': 16),
  ),
);

Perfect.

Now, if $header-sizes is still present in a mysettings.scss, the $header-styles: ... !default; is ignored and overriden with a newly generated $header-styles by help of build_from_header-sizes($header-sizes).

/// Styles for headings at various screen sizes. Each key is a breakpoint, and each value is a map of heading styles.
/// @type Map
$header-styles: (
  small: (
    'h1': ('font-size': 24),
    'h2': ('font-size': 20),
    'h3': ('font-size': 19),
    'h4': ('font-size': 18),
    'h5': ('font-size': 17),
    'h6': ('font-size': 16),
  ),
  medium: (
    'h1': ('font-size': 48),
    'h2': ('font-size': 40),
    'h3': ('font-size': 31),
    'h4': ('font-size': 25),
    'h5': ('font-size': 20),
    'h6': ('font-size': 16),
  ),
) !default;

// $header-styles map is built from $header-sizes in order to ensure downward compatibility
// when $header-sizes is depreciated, $header-styles needs to get !default values like settings.scss
@function build_from_header-sizes($header-sizes) {
  @warn 'Note, that $header-sizes has been replaced with $header-styles. $header-sizes still works, but it is going to be depreciated.';
  $header-styles: ();
  @each $size, $headers in $header-sizes {
    $header-map: ();
    @each $header, $font-size in $headers {
      $header-map: map-merge($header-map, ($header: ('font-size': $font-size)));  
    }
    $header-styles: map-merge($header-styles, ($size: $header-map));
  }
  @return $header-styles;
}

// If it exists $headers-sizes is used to build $header-styles. See the documentation.
@if variable-exists(header-sizes) {
  $header-styles: build_from_header-sizes($header-sizes);
}

@kball Note, that I have included an @warn in @function build_from_header-sizes($header-sizes). This function is only called if $header-sizes is still present. I am not sure which version is then actually depreciating $header-sizes?

@brettsmason The changes are minor. I think it would be easiest, if you include them manually in your PR by hand, before I am trying to find out how to PR to a PR. What do you think?

I am going to do some more testing now.

@karland
Copy link

karland commented Nov 30, 2016

I tested the code in the above (edited) comment and its working as expected. Would appreciate somebody else confirming this.

…ate settings file variables based on `$header-styles` rather than `$header-sizes`.
@brettsmason
Copy link
Contributor Author

@karland @kball I added the code you provided and tested it in both scenarios - it seems to work fine and generates the correct settings values, so I'm happy with it.

@kball kball merged commit f75aea2 into develop Dec 1, 2016
@kball
Copy link
Contributor

kball commented Dec 1, 2016

Merged!

@kball
Copy link
Contributor

kball commented Dec 1, 2016

@karland @brettsmason can one of you add migration notes to this PR that we can point folks to?

@kball kball added this to the 6.3 milestone Dec 1, 2016
@karland
Copy link

karland commented Dec 2, 2016

@kball Here is a suggestion for the release notes

Migration Notes:

* In order to facilitate vertical rhythm layouts, the old `$header-sizes` map has been replaced with a more general `$header-styles` map. `$header-styles` map not only allows to set the `font-size`, but also `line-height`, `margin-top` and `margin-bottom` per header size and breakpoint. `$header-sizes` still works, however, it is going to be depreciated. If `$header-sizes` is present in your `_settings.scss` it overrides any `$header-styles` map. Check out [#9419](https://github.com/zurb/foundation-sites/pull/9419)

@kball kball deleted the vertical-rhythm branch April 20, 2017 18:38
jessedoyle added a commit to amaabca/ama_styles that referenced this pull request Aug 18, 2017
* Foundation deprecated the `header-sizes` variable name with
  version `6.3`. The new name is `header-styles` and it uses a
  slightly different format.
* Fix a SASS depreation warning about extending from the compound
  selector `small.error`. This will be removed in a future version
  of SASS.

SEE: foundation/foundation-sites#9419
SEE: sass/sass#1599
jessedoyle added a commit to amaabca/ama_styles that referenced this pull request Aug 18, 2017
* Foundation deprecated the `header-sizes` variable name with
  version `6.3`. The new name is `header-styles` and it uses a
  slightly different format.
* Fix a SASS depreation warning about extending from the compound
  selector `small.error`. This will be removed in a future version
  of SASS.
* Point to a previous version of Hipchat because version `1.6.0`
  broke the OAuth flow.

SEE: foundation/foundation-sites#9419
SEE: sass/sass#1599
SEE: hipchat/hipchat-rb#202
jessedoyle added a commit to amaabca/ama_styles that referenced this pull request Aug 18, 2017
* Foundation deprecated the `header-sizes` variable name with
  version `6.3`. The new name is `header-styles` and it uses a
  slightly different format.
* Fix a SASS depreation warning about extending from the compound
  selector `small.error`. This will be removed in a future version
  of SASS.
* Revert to hipchat API version 1 because the tokens for V2
  were not properly authenticating with Hipchat and the
  hipchat gem.

SEE: foundation/foundation-sites#9419
SEE: sass/sass#1599
SEE: hipchat/hipchat-rb#202
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants