CSS: Code Smell Sanitation

Imagine this: you are reviewing someone’s CSS. Everything looks valid and has passed linting. Does that mean it’s ready for production? What are some other things you should be spotting before passing the code review?

In this talk, Fiona will show you how to spot some of the early symptoms of code smell and how you can make your CSS more robust.

CSS invites a love/hate relationship: it’s quick, easy and powerful but messy, unclear and “dark magic”.

When reviewing code, it can be hard to know what issues to look for and how to address them.

CSS linting helps in checking basic syntax, spotting errors and following predefined rules.

Build your own list of things to check for not covered by linting.

Stress test your user interface with different layouts, font sizes, screen sizes, element combinations.

CSS makes visual changes, so check how things actually look in browsers.

Developer toolbars can also spot problems.

Ultimately, be human, don’t expect tools to do your work and always question your own code.

Among the things that linting checks can be naming conventions, duplicate selectors, choice of units in sizing fonts, and a lot more.

Linting can in some cases be integrated to work with other tools to provide on-the-fly error advice.

Issues you should check for not picked up by linting can include:

  • Code that is too long, vertically or horizontally, indicating unnecessary complexity.
  • Selectors that are too long, overly specific and hard to edit later.
  • Magic numbers that “just work” without explanation.
  • Fixed heights, absolute positioning and other techniques that fail when layout changes.
  • Legacy code using superfluous old browser prefixes.

Opening title – All right.

To finish off this session, before you have a break, we’re going to have Fiona Chan.

So Fiona is probably particularly well-known up in Sydney where she is the founder and organizer, along with David Lewis, of SydCSS.

Fantastic meetup there.

She has done also engineering work on the front end and elsewhere.

She used to work with Nicole Sullivan.

And she is now a technical recruiter with Lookahead Search.

And everyone will know Mel who’s up the front who’s no longer a technical recruiter there but she’s now an engineer.

But folks like Mad Allan who kind of very much part of the ecosystem here.

And Fiona’s up there in Sydney.

She’s going to talk about the idea that basically treating CSS as software engineering and debugging and linting and then detecting code spills and just getting our CSS right. So to finish off this bracket of presentations, please welcome, Fiona Chan.

(applause) – Okay.

So I want to start my presentation with a bit of a poll.

So put your hand up if you love CSS.

Oh, cool! There seems to be about the same amount of people in Sydney and Melbourne.

That’s good.

We can all be friends.

Oh, sorry, I forgot.

Keep your hand up if you also hate CSS.

Okay, what about people who just simply hate CSS like? We can still be friends.

We can still be friends.

So, I think a lot of front-end developers have a bit of a love-hate relationship with CSS. I really love it because it can be relatively easy to pick up.

I can use it to achieve all kinds of stylings and layouts that I want these days, and it’s pretty easy across browsers as well. But I admit that sometimes I really hate CSS, too, because it may seem easy, but, oh, it is not like that at all.

I find that CSS code can get messy very quickly. Things can just sort of fall over, layout breaks, and you’d be tearing your head out, hair out, wondering what is going on.

And that may or may not be a reason why I’m not a developer anymore, because I really want to keep my hair.

(laughter) And a hear a lot of people say that CSS is a little bit like dark magic.

There’s so many ways you can write CSS.

And when it actually comes to code review time, you don’t really know what to look out for in that pull request, whether something that’s been put in and whether things are still going to work or not.

You just have no idea.

But there are definitely things that you can do and spot during your pull request to make sure that the code going into your production is actually going to be okay.

So, today, I’d like to share some of these tips with you. So, first thing to do, if you haven’t already done, is to actually set up CSS linting.

Linting is a tool that checks for problems in your CSS.

You can use it to check for basic syntax, errors, or make sure that, it can also check your style sheets against a list of predefined rules that you’ve given it. So if you’re just writing in plain CSS, you can use a tool called CSS Lint, or if you’re using a pre-processor like Sass, you can also use a tool called Sass-lint.

So these tools can be quite powerful.

It can check for things like whether your naming convention actually matches your coding standard, whether there are duplicate properties in your selectors, or when you have too many levels of nesting, it will give you a warning.

It can also, it can even check for things like if you don’t want a certain unit to be used for a certain property.

So, for example, for font size, maybe you don’t want people to be using pixel anymore and you only want them to use RAM.

Linting can sort of warn you that, if someone starts using pixels a lot.

And linting too comes with a set of different rules that I’ve mentioned.

So make sure you actually modify them so that they actually match what’s currently in your coding standard.

You can also integrate linting so that it’s part of your day-to-day process to make your life a lot easier.

So if you’re using something like Sublime, you can download a package called Sublime Linter. So what it does is that as you’re coding, and there are errors in your code, it will give you warnings so that you can go back and fix it as you go.

You can also integrate it so that it’s part of your integration process, like Travis CI, so that if you actually create a pull request and it doesn’t pass the linting check, it will give you this big red warning sign to say, “Hey, don’t merge that pull request just yet. “Go back and fix up your code.” So while linting can do a lot for us, it is definitely not bulletproof.

It is just a list of rules that we’ve given out and things like pull requests exist for a reason because we simply cannot just be relying on these two links.

So what are some other things that we can look out for when we review CSS code? First thing I like to check is the length of my CSS, both vertically and horizontally. So say this is what I see in my pull request. I can sort of scan vertically that there’s a lot of properties in there, and to me that is usually a warning sign.

We want to be building small modular components. And when something starts having too many stylings on it, we should probably start thinking, “Does it actually make sense to break these “into smaller pieces?” Another thing to look out for is how long your selector is.

So selectors that are too long become overly specific which can make them a lot harder to overwrite later on if that’s what you want to do.

And sometimes they don’t even need to be that long. So in this particular example, do I actually need the class’ card and collective? Can we just simply use card name and it will probably still work? So make sure that you ask these questions when you see something like this.

The next thing I like to look out for is what a lot of people call magic numbers. Magic numbers are basically values that just work. There are sometimes no particular reasons why these numbers work.

It just works.

And these numbers can be very brittle because things will probably work now but when you start having, in responsive web design, when your layout starts changing a lot, these numbers will probably not work anymore. It is also a lot harder for other developers to understand why you have a specific value in your code and what does it actually correlate to. So, an example where I find a lot of magic numbers can happen is in navigation dropdowns.

So here, I’ve got a dropdown that’s position absolute, and I’ve given it a value of 48 pixel.

So that 48 pixel happens to be the height of the navigation bar at this moment.

So things seem to be working fine.

That’s okay.

But then suddenly you want to change the height of your bar and you start giving your anchors more top and bottom paddings.

So now, when you actually hover over it, your dropdown is no longer positioned correctly because the bar is now more than 48 pixels. So a better way of going about it is to actually give it a top position of 100%.

So now that you can see your dropdown is positioned correctly again, no matter how tall or how short your navigation bar is, your element will always be positioned correctly.

So you’re no longer relying on that magic number and it just makes your code a lot more robust. But writing code is never quite like white, and sometimes you would need to use those magic numbers. If you do, just make sure you actually add a comment so that other people on your team will know why it’s there and how you came about working out that specific value.

Another thing to look out for that’s related to magic number as well is we need to have fixed height on elements.

A lot of the times, we get given designs where the columns are perfectly aligned, things are all equal height.

And the easiest solution might be just to put a fixed height on your columns and be done with it. So things probably look okay now, but as soon as you have a screen size that’s smaller and your content starts wrapping, you can see that your layout soon starts breaking and your content starts getting cropped off, and that’s not good.

So design can look perfectly fine on paper, but when it actually comes to implementing it, it may not be quite like that.

So a better way of going about this is to actually use Flexbox so that when you actually have a smaller device, you can see that even when the content starts wrapping onto two lines, your columns remain equal height.

I won’t go into details as to how you should be using Flexbox, but if you find that you actually need to have things that are equal height, actually look into using Flexbox instead.

Sometimes I find that people use position absolute in layouts a lot because it’s easy, we can just say, “I want this thing here, “I want this thing there,” and happy days.

But is it really the right option, or is it just an easy option? I’m always a little bit skeptical when I see position absolute being used a lot because there can be a lot of drawbacks if you’re not careful with it. So in this particular example here, I have a sidebar. It’s positioned absolute to the very right-hand side. It looks okay.

But as soon as your content starts extending, you can see that it now overlaps your footer, and that’s not cool.

So elements on the page can very easily overlap each other when you use position absolute and you’re not careful with it.

There are definitely times when you should be using position absolute, and that’s okay.

But if you do, just really make sure that you stress-test your user interface.

So test it under all kinds of scenarios, like what happens when you change your font size. What about when you have different types of content? So not just having text.

What about when you put an image in there, how will your layout behave? What about when you have different kind of screen sizes and different devices, or if you have a particularly interactive page and things get removed or added on the page, will your layout still remain intact? So really make sure that you really test out your front-end build.

Another thing to really watch out for is when you have values being set and then unset later on.

So what I mean by this is that as soon as you need to remove styles that you have previously added, that’s usually a warning sign.

So here I’ve got a base box component.

It’s got padding.

It’s got background, border, and box shadow on it. It’s all good.

But now I want to extend this base box component and create two other types of boxes.

So, here, I’ve got another box that doesn’t have any background colors or box shadow on it.

And then I’ve got a second box that doesn’t have any borders on it.

So here you can see that I’ve originally set a background and a box shadow and borders on the original box.

And then, with the other boxes, I need to start removing them.

So that just seems a little bit ineffective that you’ve added styles in one place and then later on you need to remove them.

So a better way of going about this is to just have your base box component with just bare minimum styling, so in this case, the padding.

And then when you create the two other boxes, that’s when you start adding those extra styles on top of it.

So now you can see that I’m no longer giving my box a background color and then removing it later on. And it just makes your code a lot cleaner and it can also really cut down on the number of lines of CSS you need to write as well.

And during pull request time, we sometimes come across that code that’s been lingering in that code base for a very long time.

You might find that you have old browser prefixes from the days when you still need to support IE6 or IE7.

So make sure that you’ll always be checking what browsers your website is currently supporting. And when you actually come across really old browser prefixes, you can actually go and remove them and start cleaning up some of that code.

And even if you use a tool like Autoprefixer where it automatically adds the prefixes for you, make sure that every now and then, you go into that config file and check that in the config, it’s just supporting the browsers your website needs to support, and so that to make sure that your Autoprefixer is not generating unnecessary prefixes for you.

And CSS changes are visual changes, so it is a very good idea to actually check out the pull request and then bring up the front-end build in the browser and then review it that way.

When we’re just reviewing the code in the pull request, we can see that, “Oh, it looks about valid.” But how do you actually know that that’s actually looking okay in the browser? So if you start building something that’s quite complex or if you’re building like a new component or something that involves really big changes to look and feel, it is a really good idea to check out the pull request and look at the front-end build in the browser. And after you check out the pull request, you can use the browser’s developer toolbars to inspect the changes, inspect the new component so that now you start getting a much better understanding of what these changes actually are.

So say this is what I see in my pull request. Just by looking at it, I can vaguely tell that, “Okay, some kind of overlay.

“I don’t really know where it’s used.” So if I don’t really know where it’s used, how will I know that the CSS properties that’s been put in here are actually the correct ones that I should be using? But if you actually bring it up in the developer toolbar, now I can see that, “Okay, the overlay is actually “over this image, and it only happens when you hover “over the image.” Now you’re just getting a much better context of what the changes actually are and whether the CSS being used actually makes sense here. And, like I mentioned before, one thing to be aware of is when you have values being set and then unset later on.

So the developer toolbar is an excellent way where you can actually spot these unnecessary overwrites. So here I can see that in the summary component, the padding properties got striked out, meaning those values got overwritten in other places. So make sure you actually use the developer toolbar to help you spot these warning signs.

And something that linting definitely cannot check for us is whether the code actually makes sense.

Will you actually be able to maintain this code later on? So when you’re reviewing the code, make sure you check for things like whether the naming of the classes makes sense or you actually need some kind of documentation to tell you what these class names are.

What about the way the CSS is organized? Is it easy to follow or will you need to go and find the original developer and ask, “Hey, what’s going on here?” Also the properties that are being used.

Should you be using position absolute or should you not be using it? Should you be using floats instead of Flexbox? So make sure you actually check for these things. So there are lot of things that linting can do for us, but after all, they aren’t humans, and we are the ones who actually need to understand the context of the change, understand the code so that we can work with it later on.

So when you’re reviewing CSS, make sure you actually look for things like magic numbers.

If you start seeing a lot of specific values, start asking why there are so many of these values there. If you have fixed height, make sure that you’re using maybe Flexbox instead of just putting a fixed height on it. Or if you’re using position absolute a lot, why they’re there.

Should you be using them at all? And then there’s also values that are being set and then unset later on.

If you start seeing them a lot happening, make sure you go in and maybe look at refreshing some of that code.

And also, make sure you don’t forget to actually check how that front-end build in the browser because that can tell you a lot when you’re reviewing someone’s code.

And after all, make sure you check whether the code is actually easy to understand.

So I hope these tips will be helpful to you and they will give you a lot more confidence next time you actually need to review someone else’s CSS.

Thank you for listening.