Typography: change the unit measurement from 'em' to 'rem' #174
Conversation
|
@sumedh123 just changing the unit will not suffice in our case. It's not so much change the unit that we wish to do, but change the approach entirely. This will most probably mean a major code change and a change/removal of the em mixin we are using |
| @function em($value, $context: map-get($typography-font-default, font-size)) { | ||
| @return ($value / $context * 1em); | ||
| @function rem($value, $context: map-get($typography-font-default, font-size)) { | ||
| @return ($value / $context * 1rem); |
There was a problem hiding this comment.
We should reconsider this approach of deciding in pixels and then converting to ems/rems. It should be the other way round, deciding a scale then converting it to pixels(only if necessary). I'll bring it up in next meeting. We can keep the PR pending until then.
| font-size: em( 16px ); | ||
| padding: em( 4px, 16px ) em( 8px, 16px ); | ||
| font-size: rem( 16px ); | ||
| padding: rem( 4px, 16px ) rem( 8px, 16px ); |
There was a problem hiding this comment.
Padding should probably be em not rem. Again a point we can discuss during next meeting. Padding is meant to be space between border and text to make text look better and readable, it makes sense to decide padding in scale of current element font size rather than root element font size.
|
The changes I requested all need discussion, but they need to be discussed before we merge any more PRs |
I tried to refactor the code and changed the em converter to rem since it would be better to use rem. em would cause problems when 2-3 elements are nested. Opinions?