.h1, .h2, .h3...Hum... I taked a look at typography, and I think it needs a big refactor:
@mixin.About $header-styles:
So I have several suggestions to factorize and simplify stuff:
1/ For default properties, it should be applied _before_ we process the styles map, with a base mixin or placeholder for all headers, not here.
2/ We can sort $header-styles by _name_ then _breakpoint_ (_header_ > _breakpoint_ > _properties_) to have a _component_ approach, and provide a mixin to generate single responsive and non-responsive headers.
...
$header-styles: (
'h1': (
small: ('font-size': 24),
medium: ('font-size': 48)
),
'h2': (
small: ('font-size': 20),
medium: ('font-size': 40)
),
...
) !default;
...
```scss
// Generate styles for a header
@mixin header-style ($properties) { // ideally zf-header--style
... // like in scss/typography/_base.scss:330
}
// Generate header styles for each breakpoint in $map
@mixin header-responsive-style ($map) { // ideally zf-header-responsive--style
@each $bp, $v in $map {
@include breakpoint($bp) {
@include header-size($v);
}
}
}
// Instead of scss/typography/_base.scss:330
// Generate default header styles
@each $header-name, $v in $header-styles {
#{$header-name} {
@include header-responsive-style($v);
}
}
[scss/typography/_base.scss:330](https://github.com/ncoden/foundation-sites/blob/db1e825355925eb57aaa473995b6b848957fd9cc/scss/typography/_base.scss#L330)
**3/** We could factorize the handling of `$header-styles` into two mixins. `-zf-responsive-map` is not very useful in this case because we have process each map or properties (see below), but it could be useful elsewhere.
```scss
// Generate the given map of properties
@mixin -zf-properties-map($map) {
@each $name, $value in $map {
#{$name}: #{$v};
}
}
// Generate the given responsive map: write a map of
// properties or a single properties for each breakpoint
@mixin -zf-responsive-map($map, $property: null) {
@each $bp, $v in $map {
@include breakpoint($bp) {
// the value is a map of properties
@if typeof $v == 'map' {
@include -zf-properties-map($v);
}
// the value is a property value
@else if $property !== null {
#{$property}: #{$v};
}
}
}
}
4/ For the units: we expect some defined properties to always have a particular unit. Instead of defining it _just before generating the properties_, we could simply apply _filters_ on the property or on the map of properties.
// Iterate on a map of properties on apply filters on the matching properties
@function -zf-properties-map-filter ($properties, $filters) {
@each $property-name, $property-value in $properties {
@each $filter-property, $filter-func in $filters {
@if $property-name == $filter-property {
// if arguments was given, call the filter with it
@if type-of($filter-func) == 'list' {
$func: nth($filter-func, 1);
$args: nth($filter-func, 2);
$property-value: call($func, $property-value, $args...);
}
// else, call the filter with only the property value
@else {
$property-value: call($filter-func, $property-value);
}
}
}
$properties: map-merge($properties, ($property-name: $property-value));
}
@return $properties;
}
So all that would be used like this (instead of stuff in scss/typography/_base.scss:330):
@mixin header-style ($properties) {
$properties: -zf-properties-map-filter($properties, (
'font-size': 'rem-calc'
'line-height': ('unitless-calc', ('1rem')),
));
@include -zf-properties-map($properties);
}
What do you think ? (poke @brettsmason @andycochran @kball)
Pouf :boom:. Difficuly - Advanced. Because I have high expectations.
Broadly speaking, I think this is a good direction. I think this issue has somewhat metastasized into a few different things that maybe could be split out from each other into either multiple issues or at least possibly independent PRs
Does that seem right?
I do not understand 5. How would it defer from the current @mixin foundation-typography ?
Also, what do you mean by definitely could use it ?
Ah, perhaps I misunderstood your point further above:
- There is no one single @mixin.
foundation-typography is fine with me if it's working for you. by definitely could use it I mean that the current implementation... is a little heavy and could use refactoring into the cleaner and more functional approach you outline above.
Should I make a PR with the stuff above ? I think it is a little overengeenering. But I do not see an other way to handle a map of properties. At least it is generic :)
I think it would be useful to have a set of functional primitives available to work with scss maps... the _filtering_ you describe to me sounds like what I would traditionally call _mapping_ (though I admit that language is confusing when what I would think of as a _hashtable_ is called a _map_ in scss). From that perspective, it doesn't seem like overengineering but rather introducing a useful general purpose abstraction.
I've worked on it and the result is:
h1 {
font-size: 1.5rem;
}
@media print, screen and (min-width: 40em) {
h1 {
font-size: 3rem;
}
}
h2 {
font-size: 1.25rem;
}
@media print, screen and (min-width: 40em) {
h2 {
font-size: 2.5rem;
}
}
(...)
I don't know how to factorize it without generating styles by breakpoint _then_ by header :sweat:
Well if you're adding functional methods you could have the breakpoint addition happen in a function instead of at write time, then add a group_by, so conceptually this would look something like
hash |> map (transforms) |> map (breakpoints) |> group_by(breakpoints)
e.g. something like
$transforms = (
'font-size': 'rem-calc'
'line-height': ('unitless-calc', ('1rem')),
)
-zf-map-group-by(-zf-map-breakpoints(-zf-map-filter($properties, $transforms)), 'breakpoint')
I do not understand. Can you give an example of how the map would be after each function ?
With a start map like:
$header-styles: (
'h1': (
small: ('font-size': 24),
medium: ('font-size': 48),
),
...
);
Hmm... on thinking about it more I think it makes sense to group by and then map into generated scss, and what I was thinking about as a group by when we have a nested hash like this is essentially a fancy inversion... lets call it a 'nested inversion' maybe? So it would look something like...
$header-styles: (
'h1': (
'small': ('font-size': 24),
'medium': ('font-size': 48),
)
'h2': (
'small': ('font-size': 22),
'medium': ('font-size': 40)
)
);
|> map (transforms)
(
'h1': (
'small': 'rem-calc(24)'
'medium': 'rem-calc(48)'
)
'h2': (
'small': 'rem-calc(22)',
'medium': 'rem-calc(40)'
)
)
|> nested_inversion
(
'small' : (
'h1': 'rem-calc(24)',
'h2': 'rem-calc(22)'
)
'medium': (
'h1': 'rem-calc(48)',
'h2': 'rem-calc(40)'
)
)
|> -zf-code-from-map
h1 {
font-size: rem-calc(24);
}
h2 {
font-size: rem-calc(22));
}
@media print, screen and (min-width: 40em) {
h1 {
font-size: rem-calc(48);
}
h2 {
font-size: rem-calc(40);
}
}
I understand the advantages of this approach. Using a map of properties let us moving and reorganizing it. But in the same time, I want to "Keep It Simple, Stupid".
It's why I was taking about a special object "map of properties" (...properties...). All the logic is keeped inside. And a "nested inversion" would be only a way to generate a _set of responsive maps_ ( (breakpoint: (...properties...), ...), ...) in an efficient way, by factorizing them.
Yeah...
Curious what @brettsmason and @karland think as they were the ones who did the recent work on $header-styles
I am reading through this thread and as far as I understand it it deals with three issues:
_settings.scssSo, I am trying to address these issue one after another.
1. Change of user interface in _settings.scss
Let me summarize my understanding.
The current solution is:
$header-styles: (
small: (
'h1': ('font-size': 24),
...
),
medium: (
'h1': ('font-size': 48),
...
),
) !default;
@ncoden, you are suggesting:
$header-styles: (
'h1': (
small: ('font-size': 24),
medium: ('font-size': 48),
),
...
);
The current solution was chosen, based on a discussion in Issue #8164.
Also, we wanted to be downward compatible to the approach used in the previous $header-sizes map, which was structured by breakpoint. I like the current solution better, especially if one adds the other properties as described here. But this is a matter of taste I suppose:
$header-styles: (
'small': (
'h1': ('font-size': 24, 'line-height': $header-lineheight, 'margin-top': 0, 'margin-bottom': $header-margin-bottom),
'h2': ('font-size': 20, 'line-height': $header-lineheight, 'margin-top': 0, 'margin-bottom': $header-margin-bottom),
...
),
'medium': (
'h1': ('font-size': 48, 'line-height': $header-lineheight, 'margin-top': 0, 'margin-bottom': $header-margin-bottom),
'h2': ('font-size': 40, 'line-height': $header-lineheight, 'margin-top': 0, 'margin-bottom': $header-margin-bottom),
...
),
...
);
2. Code refactor
I agree, the current code looks a little heavy. It is pretty much based on the previous code base. $header-styles is a new feature, that was just recently introduced. I believe in a stepwise improvement in order to reduce complexity: new functionality first, then code improvement second. So, code improvement with respect to making it more modular and component-like is good, but the issues of change in functionality and change in code should be seperated in my view.
3. Additional functionality
This thread started of with suggesting
- Add classes for the default headers, like .h1, .h2, .h3...
Personally, I believe this is useful.
4. Questions
@ncoden You are writing
About
$header-styles:
- It does not have value units and should apply them automatically
- It should apply default properties.
I am not sure what you mean by this, as I think $header-styles does both as it stands right now unless I am misreading you. Could you clarify? Thanks.
@karland
1. Change of user interface in _settings.scss
I don't think the current solution is better. To add other properties, you can do:
$header-styles: (
'h1': (
'small': ('font-size': 24, 'line-height': $header-lineheight, 'margin-top': 0, 'margin-bottom': $header-margin-bottom),
'medium': ('font-size': 48, 'line-height': $header-lineheight, 'margin-top': 0, 'margin-bottom': $header-margin-bottom),
...
),
'h2': (
'small': ('font-size': 20, 'line-height': $header-lineheight, 'margin-top': 0, 'margin-bottom': $header-margin-bottom),
'medium': ('font-size': 40, 'line-height': $header-lineheight, 'margin-top': 0, 'margin-bottom': $header-margin-bottom),
...
),
...
);
I do not see any difference. But beyond that personal preference, the main advantage is it is sorted by _components_. You have, for each component, its own responsive behavior. You can work on the list to add or remove headers, or iterate on it to generate it.
The issue is I care about the generated CSS and I would like to avoid to duplicate media queries. But it's a question of implementation and optimization.
2. Code refactor
I saw that $header-styles is a new features, it is why I propose these changes, we do not have to handle backward compatibility. _And I already updated build_from_header-sizes to make it generate the new map.
A agree with you we should separate the change of functionalities and the refactoring. I will open an other PR for the refactoring, but for now I need to know what we should do and if it is possible and not overengineered, even with a refactoring after.
3. Additional functionality
I agree
4. Questions
By _should_, I mean "It actually does, and if we would have to reimplement it in a different way, it _ should_ keep these features".
Also, I thought about it a lot, and did not find a better way to generate headers.
In any case, we should provide a set of mixin to let the user generate its own headers (where, when and how ze wants to). The question here is how should we generate the _default_ headers.
The simplest proper way would be:
@include breakpoint(medium) {
h1, .h1 { @include header-styles(...$settings...); }
h2, .h2 { @include header-styles(...$settings...); }
(...)
}
(...)
... because of it would be simply manually generating a set of _components_ with their responsive behavior. But it would mean we assume the number of headers and their names. I don't know if it is something we can our cannot do, it is a set of _default_ headers.
If we want to factorize it, it leads us to almost where we are. The only question remaining is: should we consider font-size, line-height as _settings_ (it is what we does currently, and It mean we have to handle them manually), or as _properties_ (it is what I proposed above, and it add a sort of new type of map, and a set of function to handle them) ?
Would be cool we could add color and font-weight to the map in settings.scss as well! Thoughts?
@phifa: color and font-weight does not need a specific unit. This is why these properties wasn't in the map, we could not add a special case for each single property.
But by switching to a "map of properties + filter" approach, you can add all the properties you want in the map.
sounds cool?
I really love the $header-styles map!! Can we do the same thing for paragraph? Or some way to set the paragraph font size for small devices in the settings file? Also still, font-weight, color, etc. would be fantastic if we'd get them into the map :) !! feedback? thoughts?
@karland @ncoden Thoughts on the request by @phifa ?
@phifa I am glad, that you like the $header-style-map.
Actually, I think you have a good point. $header-styles could well be generalised into $styles, where an extra line would be added. Is this what you suggest? I do not like the idea of adding color, though.
$styles: (
'small': (
'h1': ('font-size': 24, 'line-height': $header-lineheight, 'margin-top': 0, 'margin-bottom': $header-margin-bottom),
'h2': ('font-size': 20, 'line-height': $header-lineheight, 'margin-top': 0, 'margin-bottom': $header-margin-bottom),
'h3': ('font-size': 19, 'line-height': $header-lineheight, 'margin-top': 0, 'margin-bottom': $header-margin-bottom),
'h4': ('font-size': 18, 'line-height': $header-lineheight, 'margin-top': 0, 'margin-bottom': $header-margin-bottom),
'h5': ('font-size': 17, 'line-height': $header-lineheight, 'margin-top': 0, 'margin-bottom': $header-margin-bottom),
'h6': ('font-size': 16, 'line-height': $header-lineheight, 'margin-top': 0, 'margin-bottom': $header-margin-bottom).
'p': ('font-size': 16, 'line-height': $paragraph-lineheight, 'margin-top': 0, 'margin-bottom': $paragraph-margin-bottom)
),
'medium': (
'h1': ('font-size': 48, 'line-height': $header-lineheight, 'margin-top': 0, 'margin-bottom': $header-margin-bottom),
'h2': ('font-size': 40, 'line-height': $header-lineheight, 'margin-top': 0, 'margin-bottom': $header-margin-bottom),
'h3': ('font-size': 31, 'line-height': $header-lineheight, 'margin-top': 0, 'margin-bottom': $header-margin-bottom),
'h4': ('font-size': 25, 'line-height': $header-lineheight, 'margin-top': 0, 'margin-bottom': $header-margin-bottom),
'h5': ('font-size': 20, 'line-height': $header-lineheight, 'margin-top': 0, 'margin-bottom': $header-margin-bottom),
'h6': ('font-size': 16, 'line-height': $header-lineheight, 'margin-top': 0, 'margin-bottom': $header-margin-bottom),
'p': ('font-size': 16, 'line-height': $paragraph-lineheight, 'margin-top': 0, 'margin-bottom': $paragraph-margin-bottom)
),
...
);
Yes brilliant. And why not have font weight, text transform etc. in there?
I think the motivation behind $header-styles is the need for differing layouts (size & white space) at various screen sizes. It seems less likely (e.g.) that an <h2> needs to be bold/red at one screen size and italic/blue at another. If I'm understanding $header-styles correctly, maybe it isn't the most semantic name? However, I do understand the desire for responsive type styles. I'm just not certain where you draw the line between which styles need to be responsive. It'd be messy to have every var in typography/_base.scss in a map.
Hi, I have Foundation 6.3.0 and I would like to add color or other styles to the header-styles. Since it is not working for anything else but font-size, is there another way to do this without writing new mixins or other logic?
I think generalizing $header-styles would be great. I use the precise use case @phifa mentioned on 90% of the sites I build. Mostly it's 16px for smaller screens and 18px for desktop.
I do not believe colors belong there though. I've used that before but it just caused a lot of headache with designs that use background colors on some elements. I feel colors most often relate to the background color rather than the screens size. Nowadays I use %u-bg--X placeholders to add a background color and then have a $background-palette map where I set colors for content, links, buttons etc.
Geez, I just came across this issue because I was trying to figure out how to get font-weight into the $header-styles map and was getting frustrated.
One thing to note, if you do add other styles to the map, then maybe don't need the variable $header-font-weight?
Most helpful comment
Pouf :boom:.
Difficuly - Advanced. Because I have high expectations.