Hacker News Reader

Comment by bryanrasmussen | Hacker News Reader

so another story here:

code review mandatory at last place I worked, the place had a policy of any code you push in that does not match the guidelines must be changed. Guidelines was that no em allowed, must use rem. Old part of codebase assigned to me, I found some things in CSS I improved (reuse of code, a small overflow bug)

Got comment - you need to change em to be rem.

I can't do that because there is em all over the place and this can have side effects. The diff shows it as my code but if you look you can see it is actually the old code just moved to right place.

Those are the rules fix them.

since this code was in a big release of stuff and none of our stuff was allowed through unless I changed one em to rem even though I could not be sure there wouldn't be problems I went ahead and rolled back my changes.

fixed overflow bug other way. no code reuse ever in that part of codebase.

View on HN

Yikes! Demanding a bug fix also remediate unrelated, extant style violations is a good way to ensure nothing improves.

right. I said that to him and to the frontend lead for the whole organization (very big place) - codebases this big you can't have that requirement, do it with a small codebase/team who all sit in the same room and know what's going on sure. 10-11 teams spread around with lots of different programmers, many also remote, it can't be done. But no dice.

In fact I had people specifically recommend me at times, don't do that because when it hits review with reviewer X it will be a problem. Sometimes you had to butter them up asking their opinion before (and that wouldn't always work because sometimes they would change their mind) anyway I better shut up and get to work, I can feel the bitterness returning!

There is something to be said for continuous improvement (my company has a policy of leaving the campground neater than when we got there, which is one of the reasons I enjoy working there so much), but there is a time and place for everything...

Not that this applies to your particular situation of course but I have seen places where that duplication was actually warranted - especially in css. E.g. some code was extracted and reused when it was not meant to, just drying the code up, but those things turned out not to be the same thing and it was rather painful when we had to change them independently.

As to your particular case - I’m sure the politics of the situation were pretty bad but I’ve solved situations like this by having two separate commits/prs one that uses the old style to fix a problem so the change is obvios, the other to change all the old style to the new style of code. Helps the reviewer understand what’s happening and move the PRs along. Though this was in a much more liberal code review environment of course ...

no, it would not have worked in this situation. the problem was really the reviewer who could not be gotten around.

Did you consider fixing "em -> rem" (in a separate commit) before you start applying your own fix to that code?

My CSS-foo is fairly weak; what problems would em cause such that they disallow it in the code base?

em is relative to the font size of the nearest parent thus you can have situations where your font size of an element node is radically different due to the parent it is placed in. This can of course be powerful if used correctly, but it takes someone really good to use it correctly and perhaps it shouldn't be done in big teams because big teams imply some people not very good in some particular discipline.

on edit: adding this link https://j.eremy.net/confused-about-rem-and-em/

Thanks! I never understood this difference between em, rem, px.

I learned something very new and I have been doing web development (backend mostly) for a long time.

Both em and rem refer to element sizing. Rem is relative to the base page font size, while em is relative to the containing element font size.

The consequence is that nested em specifications are cumulative; 1.2em of 1.2 em of 1.2em gives 1.2^3, while 1.2 rem is fixed, antwhere it is used on the page.

This is not of itself good or bad, it is behaviour. Use it to desired effect.

As examples, I prefer to specify font size within elements (header, nav, aside, article, footer) in rem, font size of contained text (h1-h6, code, blockquote, pre, generally) in em, and width of text elements in em.

This allows for less CSS (most of my styles are a few dozen selectors, often less), as relative text doesn't require respecification, and content blocks are defined in terms of sensible, reader-focused, preferable widths, padding, and margins.

Much of that is personal preference: I design around article text, mostly, not wireframes or pixel-perfect layouts. The designs also tend to be robust.

Examples:

    html, body {font-size: medium; line-height: 1.4;}
    article { font-size: 1rem, width: 40em, padding: 2em 4em }
    aside, nav { font-size: 0.9rem;}
    header, footer {font-size: 0.8rem}
    h1, h2, h3, h4, h5, h6 {margin-top: 3em; margin-bottom: 1em; font-weight: bold;}
    h1 {font-size: 2em}
    h2 {font-size: 1.6em}
    h3 {font-size: 1.4em}
    h4 {font-size: 1.2em}
    h5, h6 {font-size: 1.1em}
    .callout {font-size: 2em; width: 50%; padding: 1em; margin: 1em: margin-left:0; float: left: clear both;}

Relative sizes of h1-h6 don't need to be respecified when used in header, footer, aside, nav, or .callout elements. And in callout, they're automatically scaled to 2x 1rem * factor -- cumulative application of em sizing.

With simple DOMs, this can be useful. Complex DOMs and stylesheets may benefit from direct specification via rem.

So in essence, styling a component with em can reduce reusability of components more and more as em gets introduced in various places?

It's not that clear cut.

The 'em' sizing is inherited by child elements, which can be either a feature or a bug, depending on interactions.

If you're aware of this and design for it it can be a useful feature, and enhances reusability. If you're unaware of this, or your design is complex, it's much more likely to be a bug. "Bug or not?" is entirely dependent on that context.

Oh, no! You copy-pasted code, with zero knowledge about it structure, and got caught. It's sad.

Next time use throw-away account.

Reply on news.ycombinator.com