CSS cleanup

[cjcenizal]

Goals

Let's get our styles into a more manageable state, defined by these criteria:

1. It's easy to search for (and find) the selectors you're looking for.
2. It's easy to read the properties in a declaration and understand how they map to the UI.
3. You can make changes to a declaration's styles without having to worry about specificity.

Eventually, we want our styles to adhere to our CSS style guide.

References

• Our CSS style guide
• The organization of Kibana's LESS files within the folder structure

Steps

To get our styles into a state that meets these criteria, we'll need to do the following:

Remove concatenated class names (done)

.pagination-other-pages {
  &-list {
  }
}

Declarations like these make it really difficult to find classes (in this case, pagination-other-pages-list is impossible to grep for). Let's rewrite occurrences like this to be:

.pagination-other-pages {
}

  // We can use indentation to indicate a parent-child relationship between components,
  // without affecting specificity (http://cssguidelin.es/#indenting).
  .pagination-other-pages-list {
  }

Format the LESS and introduce a linter

We can use a tool like https://github.com/mutian/Sublime-CSS-Format to format the code, and then add a linter to ensure future styles are formatted correctly.

Remove bootstrap dependency

Bootstrap provides a lot of styles which we don't use/need, and of course doesn't provide many styles which we do need.

Ultimately, we should be in complete control of all the styles in the codebase.

This will allow us to eventually remove brute force specificity overrides, like this one:

kbn-settings-advanced {
  // super specific rule to override bootstrap's equally specific rule
  // https://github.com/twbs/bootstrap/blob/1f329f8f17aa989eabc6e94bdcab93e69ef0e463/less/tables.less#L35
  .table > tbody > tr > td {
    vertical-align: middle;
  }
}

The first step of this task will be to manually copy over all Bootstrap styles into our styles/ directory, and remove the NPM dependency upon Bootstrap (and take a look at how this impacts Bootstrap-related JS dependencies, e.g. angular-bootstrap-colorpicker and angular-bootstrap).

The second step will be to reduce these styles, by combining declarations for identical selectors, removing overridden styles, and removing unused selectors.

Migrate from non-class selectors to uniquely-named classes

By migrating from element and attribute selectors to class selectors wherever possible, we'll uncouple our markup from our styles. By migrating away from Angular-specific element selectors (like kbn-settings-advanced above), we'll also uncouple our styles from JS frameworks, adding an extra level of future-proofing.

For example, even something as seemingly simple as styling button can benefit from a class like .button instead, because then you can apply this class to any element, like a div.

We can also create a Button component (either Angular directive or React component) to really yield benefits for engineers:

<!-- Then they can write this -->
<Button />

<!-- Instead of this all the time -->
<button class="button"></button>

NOTE: This requires an audit of plugin styles to make sure none of them override or "decorate" the styles in the Kibana codebase. If they do, then we'll need to document our changes to aid migration.

Replace nested selectors with class names

In an effort to reduce and equalize selector specificity, we'll replace things like this:

.pagination-other-pages-list {
  > li {
  }
}

...with this:

.pagination-other-pages-list {
}

  .pagination-other-pages-list-item {
  }

This will require updating our markup to use the new classes.

NOTE: This requires an audit of plugin styles to make sure none of them override or "decorate" the styles in the Kibana codebase. If they do, then we'll need to document our changes to aid migration.

Looking ahead

Farther down the road, let's explore topics such as:

• Migrating from LESS to SCSS or simply CSS+PostCSS

[Bargs]

If we ditch bootstrap, will we develop similarly extensive documentation for working with Kibana styles? I think one of most important parts of a framework is its documentation because it shows the "right" way to accomplish things. Without good documentation, developers tend to implement the same styles in multiple ways and consistent changes become more difficult.

This might be best left to another ticket, but I'd also like to talk about guidelines for updating styles. Even the most carefully crafted CSS tends to get trashed as soon as a design change comes along. Oftentimes design changes start on one page when it's being created or updated and to avoid far reaching impacts on the rest of the app the developer uses the type of super specific selectors you'd like to remove. How do we avoid that?

[bevacqua]

@Bargs We avoid that by shifting our thinking in how to do selectors. Instead of going deep, you need to go wide.

• Avoid .foo .bar .config
• Default styles for config would be cf-* classes, cf-container, cf-title, etc.
• Use foo-config-container, foo-config-title for example. foo- being the prefix to applying changes to config based on the foo component

[Bargs]

So if you had some markup that looks like this:

<div class="cf-container">
  <h2 class="cf-title">
  </h2>
</div>

And you want to override something in the cf-title style, would you append foo-cf-title to the class list, or replace cf-title with foo-cf-title? And if you override the title styles, should you also create a foo-cf-container? It would mean duplicating the base cf-container styles without changing anything, however I think it might solve one major problem I've run into, which is what happens if the base cf-container changes in a way that's incompatible with the overrides in foo-cf-title? By giving the foo component its own complete "immutable" copy of the base styles, it prevents changes in the global styles from breaking it.

[bevacqua]

There's two scenarios there.

In one case the component is just pulled from somewhere else like a directive, in that case you would do .foo-container .cf-title with your custom color for instance, but that's still only one level of nesting and it's pretty much guaranteed to have greater specificity than .cf-title on its own.

In the other your component is just a set of guidelines on how to style a thing, and in that case you'll prefer the cf-title foo-cf-title (or just foo-title) approach. The value in namespacing comes from being able to declare "unique enough" classes that don't clash with each other.

You only create classes for the things you want to override, and you don't copy/paste you just add an extra class name and make the modifications the parent component needs

Currently we have class names like "config" being used to configure the date picker, which is the exact opposite of what namespaces achieve. If you want to name something like config for the date picker that'd be dp-config or something, and then you wouldn't have clashing issues because the namespaces are supposed to be unique.

[cjcenizal]

@Bargs I think the answer to your question depends on whether the new version of that title is a variation on the title or an entirely different kind of title, from a UI standpoint.

An example of a variation would be a title for an information panel, which is gray by default, but is red when the information panel is displaying an error message. In this case, you will have a base cf-title class for the default variation, and you would add cf-title cf-title-error for the error variation.

An example of an entirely new kind of title would be a title for something like, metadata information. Let's say in the case the title uses a monospace font and a lighter shade of gray. I would create an entirely new title class in this case, such as cf-meta-title.

[cjcenizal]

@Bargs Let me amend that... let's first step back and look at the components we're working with, which are really two types of containers (I'll call them panels, which seems a little more UI-specific): InfoPanel and MetaDataPanel. Let's say each panel has a title and a body of text, and that the InfoPanel has that error state I mentioned.

Here's how I would structure the markup:

<div class="info-panel">
  <div class="info-panel-title">
    Title
  </div>

  <div class="info-panel-body">
    Some body text
  </div>
</div>

<!-- Error state -->
<div class="info-panel">
  <!-- Change the color to red. -->
  <div class="info-panel-title info-panel-title--error">
    Title
  </div>

  <div class="info-panel-body">
    Some body text
  </div>
</div>

<div class="meta-data-panel">
  <div class="meta-data-panel-title">
    Title
  </div>

  <div class="meta-data-panel-body">
    Some body text
  </div>
</div>

I would then write component-specific styles for each of these classes, and refrain from any shared styles, mixins, or variables. This means we don't need to worry about specificity and overrides. The only override would be info-panel-title--error overriding the color.

.info-panel-title {
  padding: 8px;
  font-size: 14px;
  color: gray;
}

.info-panel-title--error {
  color: red;
}

Avoiding shared styles will also make it easier to make changes to these components without side effects, and it also makes it easier to read a component's styles and mentally map them to the resulting UI.

[Bargs]

@cjcenizal makes sense, that's pretty much what I was thinking after @bevacqua's comment. My only question at the moment is, why use an override in the case of info-panel-title--error instead of just going all the way, repeat the styles in the error class, and remove the base class from the div when there's an error? That would avoid issues like the following scenario:

A month later someone says "just making the text red doesn't grab the user's attention enough, we need to make the error box BIGGER! Add some padding to info-panel-title-error. 3 months later someone decides there's not enough whitespace around the regular title, so they add some margin, but they don't test what errors looks like because it's a simple one line css change. Oops, now there's so much padding and margin when there's an error it pushes the actual content below the fold.

Ok, kind of a contrived example, but you see what I mean. The only downside I see to duplicating all the styles in the --error class is a little extra work to update styles across the board. But you can get the full list of updates you need to make by grepping for info-panel-title*. It might even be a good thing that the dev has to slow down and update the styles everywhere because it forces him/her to actually think about the (potentially far reaching) implications of the change rather than just overlooking it.

Or am I just nuts?

[bevacqua]

You're just nuts.

Repeated rules are the bane of maintainability when it comes to CSS.

I would give contributors the benefit of the doubt that they'll make sure when making changes to at least check if the elements they're changing won't suffer as a result.

[Bargs]

Repeated rules are the bane of maintainability when it comes to CSS.

This seems to be counter to what @cjcenizal is saying.

I would give contributors the benefit of the doubt that they'll make sure when making changes to at least check if the elements they're changing won't suffer as a result.

I was trying to be funny, but jokes aside sometimes it's not a problem of laziness on the devs part. Predicting all of the ramifications of changing a shared style can sometimes be downright impossible :/

[cjcenizal]

@Bargs If info-panel-title--error had the same properties as info-panel-title, and we extrapolated that logic to all variations of all components, then we would create three problems:

1. Most importantly, readability would suffer. Each variation class should only contain changes to the base class. If it doesn't, then it becomes very difficult to understand what it's purpose is.
2. Maintenance would also become very difficult. If we wanted to change the font-size for the InfoPanelTitle component, we would need to change it in the base class and every variation class.
3. Lastly, the compiled CSS file-size would become bloated.

[tsullivan]

We can use a tool like https://github.com/mutian/Sublime-CSS-Format to format the code, and then add a linter to ensure future styles are formatted correctly.

I'd like to suggest finding a command-line tool that can run as a post-commit hook to format the code. That way, the power to autoformat isn't just held by people who use a certain editor. But if there's a common plugin that can install into all the popular editors (such as Emmet and Editorconfig offer), that would be nicer. It would really have to be the exact same plugin though, with plugin config defined in a file in the repo (such as Editorconfig offers), or else our autoformatters would restructure the code in different ways. Even if it happened slightly so, it would add noise to the changes to review.

[Bargs]

@cjcenizal those arguments could be applied to having separate info-panel and meta-data-panel classes as well. --info and --meta-data could be variations of a theoretical base panel class. So why is duplication desired in one case, but not the other?

[cjcenizal]

@Bargs What kinds of properties would the two share via the panel class? If there isn't much overlap, then there isn't much use for a shared base class.

You can also approach it from the standpoint of the user. If the two panels don't have any common meaning from a user- and UI-standpoint (i.e. they are used to communicate very different ideas), then this is a good point at which to create two uncoupled components and base classes.

EDIT: But you are completely right, that it's possible that panel could be a component, and panel--info and panel--meta-data could be modifiers, if they form a cohesive set of classes... in other words, if info and meta-data are simple variations on the panel base class and communicate similar ideas to the user.

[Bargs]

Depends on the design obviously, but I could see them sharing quite a few. Padding/margin, flexbox properties, border styles, box-shadow, lots of stuff might fit the basic definition for a "panel".

I'm playing devil's advocate a bit here. The point I'm trying to get at is that the decision to create a variation of a component vs. a new component might not always be clear. So when I asked "why is duplication good for separate components, but not variations" I was trying to get a better feel for the difference between the two. If the decision is too subjective, different devs will come up with different solutions and we won't have much consistency. For instance, we might have info-panel and meta-data-panel with error classes as variations, and then someone else might come along and implement a generic error-panel class which would be totally reasonable but wouldn't fit the established pattern.

I do like the idea of thinking about it from the user's perspective. As always, I think it ultimately comes down to starting with a consistent design. IMO the structure of the CSS really needs flow from @alt74's design. There will always be friction If design and dev aren't thinking about things in the same terms. Which reminds me, another item on the checklist should be establishing a shared vocabulary (goes back to the point on documentation) for both high level concepts (component vs variation) and individual design elements (what's a panel? what's an alert vs a notification?).

[kimjoar]

I love that we're finally discussing clean CSS 🎉 I think the ideas in here are great. I ❤️ the initial post, CJ.

I think it'll be extremely difficult to find a way to get everyone to make consistent choices — lots of edge-cases et al. But I think a good first step is to not attempt to be DRY. The "cascading" part of CSS is ... interesting at scale. I think a good way of thinking about it could be to follow the "boundaries" on React components or Angular directives. If it's two different directives/components it might make sense to have separate css for them (and also co-locate the css file with the directive file).

I also think it'll be difficult to plan out how all of this will look (from a code standpoint). We'll need to do gradual work on this and at the same time improve our styleguide/documentation. I think we should have a short doc explaining how we want our CSS to look like. In PR's (when we get to that stage) we should be strict when reviewing the code to make sure we follow it. And we might want to get CJ to review the css of many PRs in the beginning to see normal issues etc (basically have someone who tries to push us gradually in the right direction, and who has the "entire view" of the process). As we learn more as a team, it'll be easier for everyone to see better solutions.

I think a good start would be to have a "consistent" CSS naming convention. As CJ mentions already, write out all the names, but also to use meaningful hyphens or underscores. There's some great ideas in https://github.com/suitcss/suit/blob/master/doc/naming-conventions.md and http://getbem.com/naming/, for example. I think either of these (or something else) might make it much easier to both write and read the css (and make it easier for us to stay consistent).

I'd love for us to jump to PoCs as fast as possible (and not go for the big-bang process and decide :allthethings:). Just start building tools (e.g. #7382), and start playing around with code, styleguide, and a tiny doc (than gradually evolves) explaining how we want our CSS to look like (basically along the lines of CJ's initial post above).

[bevacqua]

Wouldn't a good rule of thumb be to have somebody who's the gatekeeper of CSS and have them review every single change pertaining to CSS while they compile a styleguide ruleset that eventually does their work for them?

[kimjoar]

+1 (at least as much as possible)

[cjcenizal]

@bevacqua I will volunteer for that assignment!

[bevacqua]

@cjcenizal I didn't want to coerce you into doing it, but you were my candidate 🎉

[bevacqua]

Other than that I feel like we would keep on laying out opinions on this GH issue ad nauseum. I'd move to close it and see how it goes. Although maybe keep it open awaiting for the progressive cleanup PR.

[Bargs]

Wouldn't a good rule of thumb be to have somebody who's the gatekeeper of CSS and have them review every single change pertaining to CSS while they compile a styleguide ruleset that eventually does their work for them?

Totally agree with this. I've actually been thinking recently that this might be a good strategy to bring some consistency to all of our PRs, not just for CSS.

Other than that I feel like we would keep on laying out opinions on this GH issue ad nauseum.

But I get paid per word on GH!

[cjcenizal]

Here's a more detailed proposal on how I think we can write our styles to be maintainable and scalable: https://gist.github.com/cjcenizal/10a7651732433159c1ba18e4feedce7c

[cjcenizal]

Closing per @bevacqua's suggestion. I'll continue to use this as a roadmap.