Closed maciejpi closed 9 years ago
I don't think this is necessary. I appreciate the shortcut, but the function should allow $target
to be any type rather than force px
.
What are the other types? It wouldn't make sense writing rem(12rem) or rem(12em)
It would make sense writing rem(12em)
or rem(12vw)
or rem(12)
. The function should return rem
type or the original argument unchanged.
If you imagine when changing a line of code, does this make sense?
-padding: 12px
+padding: rem(12)
I would argue that a diff of the following makes more sense:
-padding: 12px
+padding: rem(12px)
And the inverse:
-padding: rem(12px)
+padding: 12px
In the inverse case, if it were rem(12)
, we wouldn't know immediately which type the value 12
is meant for, without reading the source of this function to find out.
There's no point in using em
when there's rem
vw
... why would you convert units that are relative to screen size to rem
? I'm actually not sure if that's possible. have you ever tried that? Doesn't work in codepen
Ok.
I don't think this shortcut will save any time or increase productivity though. And it's a pretty big change to the expected functionality. I'd argue that it's counter intuitive. What about rem()
gives the impression that it would return px
?
However, you could add a new function that does what you are asking. Perhaps in the future we may find that the new one is used more, and then we can consider deprecating this function.
I have to say I would rather keep the px
too, it's just clearer to me. Also adding rems would be more of a hassle if I have to go through and delete all the px
s as well :)
You wouldn't have to delete anything as it would work with and without px
. but sure we can keep it as it is. not a big deal:)
px
also makes it very clear what is happening. Although we all may be used to seeing rem(12px)
everywhere, for a new dev looking at that rem(12)
is less clear.
Agree for keeping px
, it's not easily implied just because we know we're using px.
Closing as wontfix
. Sorry @maciejpi =)
@maciejpi ;_;
@perry OI
@incuna/frontend please review
with this small fix it would be possible to use the rem function as it is currently (i.e.
rem(12px)
) but also like this:rem(12)
which is a bit shorter