Remove (or fix) REM function

[molwells]

The font-size: 62.5%; is making our rem function wonky. We want rem(1) to equal 10px, but it currently outputs .08333px.

I vote to remove it, as I have recently joined this bandwagon: https://hackernoon.com/rems-and-ems-and-why-you-probably-dont-need-them-664b9ce1e09f

[cmalven]

There may be a reason why this is happening, but I don't think it's because the base font-size is wrong. You can test this pretty easily because most ODC sites output a font-size in both rem and px, so you can go to almost any site ODC has built and uncheck the rem font-size value in Chrome Dev tools to verify that the font-size doesn't visually change. And if a site computes 2.4rem as the same as 24px then the math must be working out so that rem(1) equals 10px.

All of that being said, I almost never use the rem(1) function, so that function might be doing something weird that makes the value come out to something wrong.

[molwells]

@cmalven Do you use the rem function in your $spacing map (_variables.scss)? It's wonky there too, and I often have to do stuff like this:

[cmalven]

@molwells

Looking back through some recent projects…  doesn't look like I almost ever use the rem() function. My spacing map values are all straight-up rem values (e.g. 2.5rem) which get converted to pixels via the pixrem in our Gulp styles task.

Only one thing I can think of that might be causing an issue:

• The rem() function looks like it's output is based on a $em-base variable for the project. If this isn't set correctly for your project it could throw the whole thing off.

To test this, I spun up a new project using the generator and set a spacing value to rem(50). You were right, when I looked at the output it was rendering as margin-top: 4.16667rem;.

Then I added a new line to _variables.scss of $em-base: 10px; After that, the code correctly renders margin-top: 5rem;.

So to fix this we need to do 1 (or 2) things:

• Add an $em-base: 10px; to our _variables.scss file in the generator.
• Change the default $spacing map to use straight-up rem units instead of using the function. (e.g. 5rem vs. rem(50). I don't see any reason we ever needed to use the rem function here to being with.

Footnote:

I'm focusing on the easiest way to fix this issue, but I think the use of rem vs. px units is a valid debate. I've felt for a pretty long time like rem and em units usually aren't any better or more useful than px units the vast majority of the time, but I also feel like we pay such a small cost for using rem in place of px these days that it's possibly worth it to keep using rem just so if we ever needed to scale the entire site by changing the font-size on html we have the option of doing that, even if we almost never do it.

[cmalven]

Found another thing:

In _px-to-em we actually are setting a default $em-base, but we're setting it to 12px!

$em-base: 12px !default;

We should be doing something similar in _px-to-rem.scss, but I have no idea why we'd be setting either of these to anything other than 10px. If we updated this n both of those files we could probably get away with not setting this value in _variables.scss, but probably best to set it there as well.

[molwells]

Thanks for digging into this with me! I'll go ahead and open a PR to get this resolved. Brian mentioned this in #14, but we're slated to discuss the ODC sass toolkit in an upcoming dev retro. I'll bring up the rem vs px debate with the group. Using rems definitely bothers me less when it's an obvious conversion (i.e. 1rem = 10px).

[brianjhanson]

For what it's worth, I would vote to straight up remove the _px-to-rem.scss and _px-to-em.scss files. It sounds like they aren't used very heavily and are just muddying the waters.

(I suppose that's the same as option 2)