DEV Community

Cover image for How do you review CSS?
William Iommi
William Iommi

Posted on • Updated on

How do you review CSS?

What is your workflow when you have to do a tech review and there are a bunch of CSS-like files?
Do you follow some guidelines? Do you build the feature branch somewhere? Or you just close your eyes 🙈, click on the 'Viewed' checkbox and let the QA guys do the rest? 😬

👇👇👇

Discussion (12)

Collapse
lukeshiru profile image
Luke Shiru

You should check for a11y issues (bad color contrast, animations not wrapped in user preference, small font sizes, etc), or lack of usage of app defined stuff such as margins, paddings, color palettes, sizes and so on, or usage of px over other better units. You should also check that bad practices such as setting z-index are avoided.

Collapse
thexdev profile image
M. Akbar Nugroho

Why setting z-index are bad practices and should avoided? Overlay, modal, etc works with z-index isn't it?

Collapse
lukeshiru profile image
Luke Shiru

As @codingjlu pointed out, is an a11y issue. If you want to put something in top of something else, you should be using the DOM order, not z-index. Is as bad as setting tabindex in HTML.

Thread Thread
thexdev profile image
M. Akbar Nugroho

Oh, thank you. I think I need do research about a11y and z-index

Collapse
codingjlu profile image
codingjlu

z-index is bad for accessibility (like screen readers) because it sort of places stuff in the wrong order and confuses people.

Collapse
codingjlu profile image
codingjlu

But at the same time, remember that z-index is useful in some situations like popups or banners or nitty gritty stuff.

Collapse
eugenman profile image
Eugen

Branch of frameworks and libraries working with tabindex and z-index.

Collapse
robie577 profile image
Robin Philip Thomas

For us, first round of reivew is done by stylelint. What I usually check for in the next round are: are they

  • reusing existing styles and design tokens,
  • semantics of the html
  • css specificity probmems (seeing !importatnt is an easy sign of bad css)
  • improper usage of %, vh,vw, rem or em
  • adding unwanted css properties (usually when copying from stackoverflow without knowing what it does)
Collapse
ben profile image
Ben Halpern

Typically the question for me is... "Does this accurately follow patterns we have established in a healthy way?". Guidelines are great, but it is hard to be absolute about certain things, you sort of need to evaluate whether the current change follows patterns that lead to better overall CSS.

@pp is that okay?

Collapse
pp profile image
Paweł Ludwiczak

I think that makes sense.

I'd also add that sometimes devs tend to sneak-in some hacks in CSS more often than in other files :D I usually try to catch these and simplify when reviewing someone's CSS code. It all eventually affects readability but also maintainability of code.

Collapse
antonfrattaroli profile image
Anton Frattaroli

Off the top of my head, these are the sorts of things that would cross my mind when reviewing a PR for a UI framework:

  1. Did they use BEM correctly?
  2. Did they apply theming correctly?
  3. Is there duplicate CSS?
  4. Is there unnecessary CSS?
  5. Did they use non-class selectors?
  6. Did they hardcode colors?
  7. If they modified the CSS, did they update the screenshots?
  8. Are the scss variables documented and marked as default if necessary?
  9. Are the rules in the correct order?
  10. Are animations able to be disabled?

Those are the only ones I can think of that are actionable. "Does it feel right?" doesn't really count, but it would be on my list.

Collapse
wiommi profile image
William Iommi Author

Thank you all for your time/responses, much appreciate ❤️.

I'd say that our workflow is very similar to the one described by @robie577 and @antonfrattaroli.

We use BEM, Stylelint, SCSS variables, mixins, and so on. Could be complex when the project is big and you have to deal with many devs with different skills/seniority.

Anyway, I think that one of the important aspects is how solid is your UI / Design System, which is maybe close to what @ben said, if I understood correctly 😬.

If you have a poor UI/DS, you are open to a variety of ‘style combinations’ (even small stuff) that are ‘perfectly fine’ from a UI perspective, but they could lead you to a difficult review process with a bloated CSS code to maintain.