Every week(ish) we publish the newsletter which contains the best links, tips, and tricks about web design and development. At the end, we typically write about something we’ve learned in the week. That might not be directly related to CSS or front-end development at all, but they’re a lot of fun to share. Here’s an example of one those segments from the newsletter where I ramble on about code quality and dive into what I think should be considered a code smell when it comes to the CSS language.
A lot of developers complain about CSS. The cascade! The weird property names! Vertical alignment! There are many strange things about the language, especially if you’re more familiar with a programming language like JavaScript or Ruby.
However, I think the real problem with the CSS language is that it’s simple but not easy. What I mean by that is that it doesn’t take much time to learn how to write CSS but it takes extraordinary effort to write “good” CSS. Within a week or two, you can probably memorize all the properties and values and make really beautiful designs in the browser without any plugins or dependencies and wow all you’re friends. But that’s not what I mean by “good CSS.”
In an effort to define what that is I’ve been thinking a lot lately about how we can identify what bad CSS is first. In other areas of programming, developers tend to talk of code smells when they describe bad code; hints in a program that identify that, hey, maybe this thing you’ve written isn’t a good idea. It could be something simple like a naming convention or a particularly fragile bit of code.
In a similar vein, below is my own list of code smells that I think will help us identify bad design and CSS. Note that these points are related to my experience in building large scale design systems in complex apps, so please take this all with a grain of salt.
Code smell #1: The fact you’re writing CSS in the first place
A large team will likely already have a collection of tools and systems in place to create things like buttons or styles to move elements around in a layout so the simple fact that you’re about to write CSS is probably a bad idea. If you’re just about to write custom CSS for a specific edge case then stop! You probably need to do one of the following:
- Learn how the current system works and why it has the constraints it does and stick to those constraints
- Rethink the underlying infrastructure of the CSS
I think this approach was perfectly described here:
About the false velocity of “quick fixes”. pic.twitter.com/91jauLyEJ3
— Pete Lacey (@chopeh) November 2, 2017
Code smell #2: File Names and Naming Conventions
Let’s say you need to make a support page for your app. First thing you probably do is make a CSS file called `support.scss` and start writing code like this:
.support {
background-color: #efefef;
max-width: 600px;
border: 2px solid #bbb;
}
So the problem here isn’t necessarily the styles themselves but the concept of a ‘support page’ in the first place. When we write CSS we need to think in much larger abstractions — we need to think in templates or components instead of the specific content the user needs to see on a page. That way we can reuse something like a “card” over and over again on every page, including that one instance we need for the support page:
.card {
background-color: #efefef;
max-width: 600px;
border: 2px solid #bbb;
}
This is already a little better! (My next question would be what is a card, what content can a card have inside it, when is it not okay to use a card, etc etc. – these questions will likely challenge the design and keep you focused.)
Code smell #3: Styling HTML elements
In my experience, styling a HTML element (like a section or a paragraph tag) almost always means that we’re writing a hack. There’s only one appropriate time to style a HTML element directly like this:
section { display: block; }
figure { margin-bottom: 20px; }
And that is in the applications global so-called “reset styles”. Otherwise, we’re making our codebase fractured and harder to debug because we have no idea whether or not those styles are hacks for a specific purpose or whether they define the defaults for that HTML element.
Code smell #4: Indenting code
Indenting Sass code so that child components sit within a parent element is almost always a code smell and a sure sign that this design needs to be refactored. Here’s one example:
.card {
display: flex;
.header {
font-size: 21px;
}
}
In this example are we saying that you can only use a .header
class inside a .card
? Or are we overriding another block of CSS somewhere else deep within our codebase? The fact that we even have to ask questions like this shows the biggest problem here: we have now sown doubt into the codebase. To really understand how this code works I have to have knowledge of other bits of code. And if I have to ask questions about why this code exists or how it works then it is probably either too complicated or unmaintainable for the future.
This leads to the fifth code smell…
Code smell #5: Overriding CSS
In an ideal world we have a reset CSS file that styles all our default elements and then we have separate individual CSS files for every button, form input and component in our application. Our code should be, at most, overridden by the cascade once. First, this makes our overall code more predictable and second, makes our component code (like button.scss
) super readable. We now know that if we need to fix something we can open up a single file and those changes are replicated throughout the application in one fell swoop. When it comes to CSS, predictability is everything.
In that same CSS Utopia, we would then perhaps make it impossible to override certain class names with something like CSS Modules. That way we can’t make mistakes by accident.
Code smell #6: CSS files with more than 50 lines of code in them
The more CSS you write the more complicated and fragile the codebase becomes. So whenever I get to around ~50 lines of CSS I tend to rethink what I’m designing by asking myself a couple of questions. Starting and ending with: “is this a single component, or can we break it up into separate parts that work independently from one another?”
That’s a difficult and time-consuming process to be practicing endlessly but it leads to a solid codebase and it trains you to write really good CSS.
Wrapping up
I suppose I now have another question, but this time for you: what do you see as a code smell in CSS? What is bad CSS? What is really good CSS? Make sure to add a comment below!
Code smell #1: The fact you’re writing CSS in the first place
Really? That’s exactly what I do for a living, 9 to 5, 5 days a week. Only CSS. In a team where the others do Typescript, Java, management… thanks to the fact that they have me dedicated to do all the CSS work…
The other code smells: are you sure you are experienced enough as a CSS developer to make these statements? I don’t agree on any of these…
More than 50 lines of CSS is code smell… really? 37000 lines of CSS, that’s code smell (and approx. the number of CSS lines in Angular JS btw).
For #1 yes, the whole point of that one is that it is a common practice to have a pattern library, even if that means using someone else’s like Material, Bootstrap, Foundation, etc.
This rule isn’t saying that it is always true, and I’m not sure it does that great of a job of really explaining itself. A well thought out system shouldn’t require the writing of very much CSS unless you are adding new features, in which case you should be fine.
As far as the rest of the rules listed here, I’d say most of them are valid as well though they suffer from the same lack of better examples. The rule about 50 lines I think might be a bit much and might vary depending on the type of system being built. After 200 lines I might start to wonder why there is so much styling going on in one file.
(just because I can’t reply Luke P., this is for him)
So you’re basically saying that we all should use Materialize, MDL, Bootstrap, Foundation or any of the thousand frameworks out there to get the job done and if we have to write extra CSS then we are basically doing it wrong, unless we are adding extra functions!? Really!?
So then, according to you, all sites should look the same, no difference between codrops, csstricks, or any other site because we should be using any of the frameworks out there without customization, right?
Agreed. If you’re using a framework with no additional style overrides or customizations, you’re making a boring site. I’m not sure what point the author is trying to make with this one.
As for #2, I’ve heard this for years. I get that in an ideal world CSS will be completely abstracted and I’ll have a reusable class for every single thing. However, I’ve never once in my career actually had a problem arise from having a class like
.support
for a particular page element. If that style needs to be extended to future areas, the code can be refactored later when it actually becomes necessary.Most of these tips are just ways to spend way too much time on CSS and make the whole process harder than it needs to be.
I disagree with the replies to Luke here. I think most of the points in this article are fair. If you’re working on a new project or new features, obviously you will be writing CSS. If you’re not, and you’re changing existing features or fixing bugs, then maybe you shouldn’t need to change any CSS (maybe). If you’re using a UI kit or design system, then this can usually work as a dev dependency and rarely updated. If you’re not and your CSS is tightly coupled to your application components, then you will likely be changing CSS as your changing any other code. As design systems are quite popular nowadays, the point is mentioned here in this article.
As for #2, I completely agree. If you want to create a class called
.support
for the container of your support page, and then use this class to style elements on the support page, then I hope you’re not working on my projects.I think you are taking my comment about style guides a little out of context. Nowhere did I say that you should just drag and drop Bootstrap into a project. With those frameworks, and even the ones we right ourselves, it can be really easy to break away from DRY and create one off instances for things that could serve better as a helper class. Before you start writing a solution you should make sure that there isn’t already a pattern or class that does what you need to do, or that it follows the pattern libraries set of standards.
If you use an already established pattern library, yes you will still be writing CSS but the point is that it cuts down the amount of instances you need to write CSS, that’s the whole point of the first rule, questioning if what you are doing already exists within the tools you have in front of you.
I’m currently working on several React applications that all use shared components, it is critical that branding is consistent across the entire ecosystem. Our components have a lot of predefined properties for things like font sizes, grid system layouts, and many other attributes to maintain a high level of consistency. As a result, I am rarely writing CSS and when I do it is for very specific instances where a one off situation exists or a new component might be needed.
It is precisely in those instances I have to stop and wonder why I am writing CSS. Is design/UX forgetting that we already have an established way of using a pattern? Did someone else already create that style in another part that I can reuse? The majority of those questions help us to tighten our design up by removing minor inconsistencies. Other times, I find out someone else already created a new rule for a style/size that I can reuse in my component.
Anytime you are working on a large code base, with several other developers, you have to practice a certain level of restraint, because otherwise you’re code base is going to get out of hand very quickly. I see this a lot with things like inline menus, buttons, and alignment of items within grids. The majority of styling on those elements can be easily rectified with a few base and modifier classes. A good UI will have a high level of consistency that requires very little if any one off tweaks.
As far as saying that applying this rules takes a lot of time, it sure does. That time however is nothing compared to the level of effort to refactor a code base that has gotten out of hand. It’s also an impact on overall performance, no matter how small the extra weight in size might be, it all still matters.
Does this rule mean that you should never ever write CSS at all? Absolutely not, it is simply a safeguard to make your more productive and spend less time refactoring.
Seriously though, who here has ever ran into trouble with a class like
.support
? Have you honestly run into a situation where your.support
styles needed to be ported to another page, and you couldn’t refactor the code in an hour or less? Would it really be worth it to spend the extra time making something 100% scalable and nonspecific just in case you needed it later, or does it make more sense to do what you need to do now, and worry about extending it if and when it becomes necessary?Keep in mind I’m not saying a super specific class like
.support
is the perfect way to do things. It definitely isn’t. But you guys are talking like it’s completely unacceptable to just get things done in a pragmatic, effective way. People are getting reeeal picky about “code smells”, ignoring the fact that just 10 or so years ago, CSS frameworks barely existed, and styling with a class like.support
would’ve been the standard way to do things. Sure, that was then and this is now. But at the end of the day I guarantee the client isn’t going to care that your code is potentially expandable to future areas that may or may not exist.Hi Robin! Nice article and really good points, although I’m not really sure how you could write a button.scss file that’s below your ~50 lines of limit. Can you maybe provide me a link of a repo you have managed the css part of? I’d love to learn more about best practices :)
#4 is just a way to define a component. Nothing wrong there, apart from the too broadly named header-class.
What I’m hearing is an argument for resets and preprocessors, and a warning about the complexity of 50 lines of CSS (by some coding standards translating to about 12 declarations).
I wonder, is this, are all these points, in the spirit of quality and manageability? How would we explain to those who focus on performance that a reset would be needed when the site—almost all sites—works as well without a reset [1]? Or to those who run large-scale sites that their 50,000 declarations [2] should be chunked in at least 1,000 components?
I support and encourage the idea to call out CSS coding issues—but we need to pay great attention that the remedies are not worse than the original troubles, and that we do not mistake features for bugs. Styling elements, to name another example, does not, per se, mean “code smell.” That is almost like saying a car is broken because it moves—if the driver doesn’t know how to ride, that may seem very true, but we all know that it isn’t.
[1] https://meiert.com/en/blog/stop-using-resets/
[2] https://meiert.com/en/blog/70-percent-css-repetition/
I thank you for taking the time to share your thoughts on your interpretation of code smells in CSS. I don’t want to take time to share my thoughts on your smells except for #1. Starting an article off by saying that we should not be writing CSS is negative. Why would we not be writing CSS? Are you saying that we should leave it up to relying on a framework to do this? Please explain in more detail why you don’t think we should be writing CSS.
Hi Rob, I don’t think the author literally meant that it’s not a good idea to write CSS. I think it was just a way of grabbing our attention and helping us to think before we start coding. When working on a large application, a lot of the time, what you need to do has already been done before and this can be leveraged. This might be in the form of a helper class or a component that has already been written. My interpretation of this point is that we should ensure that we understand the way the application is written and the conventions it follows so that we can contribute in the best possible way and help to keep it maintainable.
Thanks for putting this article together, Robin.
Although it would seem that many code smells are up for debate, it has helped a junior developer like me know what kind of things I should be looking out for – and I definitely recognise some bad behaviours in here that I can iron out at this early stage.
Great read!
what’s the solution for #4 then ? should the header be named .card-header ?
Yep, I reckon so.
Using OOCSS methodology, I would chain classes like class=”header card-header” so that default header styles are inherited, followed by custom overrides.
Also means you don’t have to nest the Sass:
I have to agree with another commenter in saying that this post, its “smells”, lacked clarity.
#1 – Writing CSS is designing. If you’ve already written your base (what you call a reset, which should never have non-reset styles) and have component elements styled from other projects that fit into the current design, then you shouldn’t need to write much new CSS apart from layout. But the implication that we must either have something pre-written to reuse or use a pre-written framework is absurd.
How do you get outside the box when you refuse to work without the box?
#3 – Again I state, reset is not base. You shouldn’t style any elements in your reset, at all, only remove styles for the purpose of setting baselines. With that said, the base should be reserved only for styles that do not more appropriately belong in another stylesheet, for a specific component, pattern, etc.
#4 – Sass code is not CSS. In CSS, this “smell” really only exists in media queries and animations, in which case the example provided is spot on.
#6 – As another commenter noted, in many cases, 50 lines of CSS is basically 12 declarations. If you break down your code into super small chunks, like button.css, then that may work. However, you can’t have a design pattern for a single element. A pattern requires more than one. While you can break it down that far, you’re really just creating a maze of files that’s more troublesome as a solution than the problem it attempts to solve. buttons.css (plural) makes more sense. And it’s as specific as designer sanity can survive in most cases.
I appreciate the article. I actually read all the way through it in spite of my reservations. New approaches are valuable, no matter where they come from. However, after reading, I believe this article should have been prefaced with your exact viewpoint. That is to say you’re not a CSS designer, but a CSS preprocessor designer, and that you likely work on an established team to build/improve an established design, not something totally new. In other words, this article is for those working to tweak designs, not create them. :)
And since I threw some shade at you for not doing it, I’ll say that I am a programmer and designer, freelance and for personal projects (long-term, revenue generating personal projects). Have a good one!
Must be a nice job you have. I, on the other hand, get to work on a site for a fortune 100 technology company.
#1. Sure there’s a library and tool set for working with the site’s SCSS. I’m not allowed access to it since I’m a contractor. (None of the contractors are allowed access but most of the work is done by contractors.) But I’m still expected to fix things.
#2. Same site puts the entire CSS is one file. Originally it was for load times. Now it’s because it’s too big to fail. If you try to use generic class names, you’re very likely to clobber some other bit.
#3. Past developers either used element names or didn’t provide class names. In both cases, we’re left with using element names. (Usually as part of a longer rule. No, we generally can’t change pre-existing html to add classes.)
#4. This shows a misunderstanding of what scss indenting is about.
#5. At this point, overriding CSS is the only way to get anything done.
#6. 50 lines? Seriously? Maybe if you only have a couple of pages. How about a site with thousands of pages? Every six months to a year, the look and feel is updated. But they can’t break the old styles because there are pages that still use them. And they can’t load just the CSS the template needs because the CMS didn’t support that 10 years ago.
Anyway, sure, if you’re making a new, small site, these are reasonable guidelines. But for a site that’s been around for decades? None of this is realistic. We’re just piling more paint on hoping nobody will notice the cesspool of code lying underneath.
Great article!
It appears like there could be some misunderstandings about the first code smell #1: The fact you’re writing CSS in the first place. I believe the key sentence here is 2. Rethink the underlying infrastructure of the CSS. It’s not that we shouldn’t be using CSS as a language itself.
Let’s say someone needs your help on a project that you haven’t worked on before. They need an input field and a button for a newsletter signup feature. Sure, you say and ask for the design files and start writing html and CSS to match the design. This is a quick fix!
What you should do is Rethink the underlying infrastructure of the CSS. Does the current project use buttons and input fields somewhere else? Maybe there is an existing contact page or a similar form page where the two elements exist already. Then you could reuse those parts by reusing the CSS class-names, and maybe without writing a single line of CSS.
Good point ronnidc, I think you’ve hit the nail on the head!
These are all guidelines so obviously they can’t all be applied in all situations ever. What I would say on #4 though is that nesting is a good way to encapsulate behaviour. If .header truly behaves differently because it is inside .card, then it is legitimate for that to be nested. This means you encapsulate all your styles for a card within one file. If you ever remove the card component, the .card .header class can go with it.
The alternative to this is to have something like .header–in-card in the header component file, but it’s difficult at first glance to know precisely what that is doing and where it effects. Would you be confident in deleting that class if you deleted the card component?
There was a comment made earlier regarding this. I think the idea would be that you would give the element the existing class and if you needed to customise it you would add an extra class. Something like this:
<div class="header header-in-card">
That way you inherit the existing styles and you can add to them or overwrite them with the rules for the new class.
#1 Only if you are not a CSS developer? As CSS developer I hope you develop the CSS that other can easily implement without writing CSS? Just adding and configuring bootstrap is not CSS development. (FE developer who does CSS, just saying CSS develop for the sake of simplicity here)
#2 This totally depends on the project. If you are writing CSS for the new york times, yes. If you are writing for a local florist, why use extreme abstractions?
#3 Also depends on the project, but mostly only style classes indeed
#4 is a code-smell? I would say it’s a best practice…
#5 Overriding CSS not good? How do you do responsive design? I agree on overriding as little as possible though
#6 50? Ever styled a functional datepicker or forms or something like that? Or do you make 500 css files for every little thing?
Kinda confused why these are code-smells…
There’s some messed up comments on this article. I agree with most points here. This is not a way to make CSS more complicated than it needs to be. These are all good, valid points.
#1 I am a CSS developer and I love it when I don’t need to write CSS. It means the CSS I’ve already written is good and can be re-used.
#2 This is extremely important. If you did a tech test during an interview process for me and used a file named support.scss or a class called
.support
to style components in your support page, I would not give you the job. Because you are styling UI components based on their content, not their purpose.#3 To me, styling elements is only acceptable as base styles (or so-called “reset” styles), as the article says. Good, valid point.
#4 This is coupled with point #5. Naming conventions that follow something like BEM standards are much better and far more maintainable.
#5 No point in repeating what I’ve just said above. But this is a good valid point.
#6 I think 50 lines is just a number that the author prefers to stick to. But the idea in general is again a good, valid point. Don’t put too much CSS in one file. Not hard to understand, or even agree with.
I made a long list of specific comments before, and deleted the comment. I feel like the biggest flaw with this article is a lack of context. If you are building a small website almost all of these are not code smells. If you are working on a huge system with multiple developers across a wide range of code base then maybe these are code smells.
The biggest issue I got from this was you can get anyone to agree or disagree with you if you are vague on your point, arguments or context.
Also, I noticed the author snuck in the phrase “application”, which implies context. In an application environment these may be code smells.
But also, most of us developers have to just live with it, especially if we are maintaining an old code base that we have limited access to, or it’s simply not cost effective to actually “fix” the code smell. Which I would argue is common in maintenance mode projects.
So a note to the author, context and clarity, most of the points of criticism here would be moot or completely different if had you clarified your source of perspective, (ie, “large/medium/small” website/application, size of development group, or even if you are working on opensource project vs closed, or corporate or doing a pro-bono job, etc…)
I thought the author did a good job of defining the context. Please see this section which was in the introduction of the article:
“In a similar vein, below is my own list of code smells that I think will help us identify bad design and CSS. Note that these points are related to my experience in building large scale design systems in complex apps, so please take this all with a grain of salt.”
Either I am going crazy, or this article has been rewritten. I am going to say since the comments here are clearly based on lack of context, that it’s been rewritten.
And I would agree the article works better now. But again, maybe I am crazy.
Code smell #0: you are using normalize or reset CSS.
Sad to see how the web tech degraded over the last ten years.
Thank you for writing this article. It has been a long time since I have read something I have wanted to engage with. I thought that the article was thought provoking with just the right amount of information and context. Really enjoyed reading it and interacting with other readers.
I think it’s worth noting that “indenting” is different than “nesting”. I’d agree that including lots of nested styles like
.content .card .header
– can slow down a site and is often unnecessary. But “indenting” can be used to make more legible easy to manage code. For instance, this scss can work well to create BEM-style css:I’d actually advise against this because it makes searching for .card–header a lot more difficult. Be explicit over concise is my preference
Yeah James I guess that could be an issue. I very seldom search as I’m usually using an inspector that gives line numbers, so I haven’t run into that.
Agree with the author on all points. Especially after reading Atomic Design that stresses the important of planning and really creating a system — it’s very easy to write mindless CSS, everyone can learn to do it in a week or so but “with power comes responsibility”. I work on a large project now that is a part of a big app, and I had to refactor my CSS several times now b/c “fixing fixes” was getting out of control. Thanks, Chris!
I am by no means an expert but here’s my take:
#1: The fact you’re writing CSS in the first place
Sorry, sometimes I need to do a quick fix to keep my job. It’s life.
#2: File Names and Naming Conventions
I chose the best I can at the moment. Then I hope for the best. It works 50% of the times.
#3: Styling HTML elements
Agree. Unless I’m overriding somebody else’s CSS I will certainly do it. Read answer to #1 now.
#4: Indenting code
WTF? . I honestly don’t see anything wrong with that.
#5: Overriding CSS
Either we like it or not, it will continue to happen to you, to me and to everyone else here. It’s life.
#6: CSS files with more than 50 lines of code in them
Wait, what? Uh… but… … uh… … wha whaaat!
Code smell #3: Styling HTML elements
The alternative to styling the button element is to create a .button class, and have the redundant everywhere in the html.
In an app where a button is always a button, with some variations on the base theme, it’s pointless and messy to have a .button class.