Conversation
tysongach
left a comment
tysongach
left a comment
There was a problem hiding this comment.
👍 to removing $base-font-size. I always found it awkward and unneeded for a couple reasons:
- The
base-nomenclature falls down in a larger system. htmlis the root document, so why not set a “base” font size there and all other relative units can calculate off of it. This removes the need for a variable.
core/_typography.scss
Outdated
| @@ -1,7 +1,7 @@ | |||
| body { | |||
There was a problem hiding this comment.
While we are changing core font sizing, I think it’d be great to swap body here for html. This would allow any usage of rem units down the road to pick up on any default font size changes.
core/_typography.scss
Outdated
| color: $base-font-color; | ||
| font-family: $base-font-family; | ||
| font-size: $base-font-size; | ||
| font-size: modular-scale(0); |
There was a problem hiding this comment.
Have we thought about setting this as 100%? Percentage units here ensure that font sizing is scaled proportionally and evenly when a user adjusts the default text size within browser/OS settings, improving accessibility.
More info here: http://kyleschaeffer.com/development/css-font-size-em-vs-px-vs-pt-vs/
core/_forms.scss
Outdated
| display: block; | ||
| font-family: $base-font-family; | ||
| font-size: $base-font-size; | ||
| font-size: modular-scale(0); |
There was a problem hiding this comment.
This goes against convention, but I’d suggest we set this to 16px, as it prevents a zooming bug on iOS. When set anything but 16px, iOS zooms the page when an input is focused.
|
@whmii Can we move this PR forward? |
|
@whmii Ping! |
whmii
force-pushed
the
whmii-modular-scale
branch
from
e0a94e4 to
e845781
Compare
|
@tysongach Updated based on comments |
tysongach
left a comment
tysongach
left a comment
There was a problem hiding this comment.
Looks good. Mind updating the change log?
whmii
force-pushed
the
whmii-modular-scale
branch
from
e845781 to
27a46d2
Compare
|
|
||
| ### Removed | ||
|
|
||
| - Removed `$base-font-size` in favor of more specific implimentations specific |
There was a problem hiding this comment.
implimentations → implementations
|
|
||
| ### Removed | ||
|
|
||
| - Removed `$base-font-size` in favor of more specific implimentations specific |
There was a problem hiding this comment.
Cut the second specific?
ghost
mentioned this pull request
2 participants
$base-font-sizefeels a bit legacy to me since we are already usingmodular-scale(), especially, the defined behavior is to output 1em (aka modular-scale-base).no visual change.