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

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)

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants