Typography: Add Variables #139
Conversation
| @@ -54,7 +54,7 @@ h6 { | |||
| font-size: em( 16px ); | |||
| font-weight: 600; | |||
| line-height: 1; | |||
| text-transform: uppercase; | |||
| text-transform: uppercase; // FIXME Is this opinionated? | |||
| } | |||
There was a problem hiding this comment.
@sfrisk I kinda don't link this. Isn't that opionanted? More people would use h6 with normal capitalization than uppercase.
There was a problem hiding this comment.
That should probably be moved to a variable - so they have the ability to set that.
There was a problem hiding this comment.
@sfrisk I don't think something like text-transform on a heading should be a variable.. we would have too many variables that way. h1-text-transform....to ...h6-text-transform. I think if we leave that property to default, users can overwrite with css easily?
There was a problem hiding this comment.
I know there was discussion at one point about how we intend to have Chassis be used as a library/framework - if updates to the code could easily be done within Chassis itself - or if the expectation was that any modifications should be done from outside of Chassis, and Chassis's code should be left intact. If the latter - we probably want to make it a variable to avoid a lot of overwriting of styles. A plus side for making it a variable though is that a designer could better leverage the themeroller in customizing how Chassis looks.
There was a problem hiding this comment.
okay, then ill have that variable only for h6 and h5, we dont want the entire stylesheet in the variables
|
@sfrisk, @arschmitz , @kristyjy , I have left TODOs on things i am already planning to change. Please review the diff /existing typography styles and suggest what more can be moved to variables. |
0b85976 to
1fd40d8
Compare
| @@ -78,7 +78,7 @@ blockquote { | |||
|
|
|||
| pre { | |||
| padding: 16px 20px; | |||
| background: #f7f7f7; | |||
| background: #f7f7f7; // TODO replace with color variable | |||
| * ========================================================================== | ||
| */ | ||
| @mixin heading($style) { |
|
Looks good to me. @kristyjy, @arschmitz ? |
| @@ -78,7 +65,7 @@ blockquote { | |||
|
|
|||
| pre { | |||
| padding: 16px 20px; | |||
| background: #f7f7f7; | |||
| background: map-get($pre,background); | |||
| font: normal 12px/1.4 $monospace; | |||
| } | |||
There was a problem hiding this comment.
Shouldn't we handle overflow here as well ?
|
A couple of helpful additions could be :
|
Consider not messing around with font-smoothing: Please Stop "Fixing" Font Smoothing |
|
I wonder whether this Android 2.3 bug report regarding |
|
@cvrebert. Agreed. Im totally against using font smoothing and test-rendering both.. |
|
@sfrisk Updated. Can go in now once you review |
Work in Progress