Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[docs] Add css style guide section on open/closed principle #12276

Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 108 additions & 0 deletions style_guides/css_style_guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
- [Don't use multiple modifier classes together](#dont-use-multiple-modifier-classes-together)
- [How to apply DRY](#how-to-apply-dry)
- [Compelling reasons for using mixins](#compelling-reasons-for-using-mixins)
- [The Open/Closed Principle](#the-openclosed-principle)

## Selecting elements

Expand Down Expand Up @@ -103,6 +104,8 @@ Our CSS naming convention is based on BEM:

* [BEM 101 (CSS Tricks)](https://css-tricks.com/bem-101/)
* [Getting your head around BEM syntax (CSS Wizardry)](http://csswizardry.com/2013/01/mindbemding-getting-your-head-round-bem-syntax/)
* [CSS for BEM (bem.info)](https://en.bem.info/methodology/css/)
* [CSS Guidelines](https://cssguidelin.es/)

## Concepts

Expand Down Expand Up @@ -461,3 +464,108 @@ border-radius changes for all buttons, you only need to change it there. Or if
the designers anticipate that all new types of buttons should have the same
border-radius, then you can just extend this mixin when you create a new button
base class.

### The Open/Closed Principle

References:

* [The Open/Closed Principle on bem.info](https://en.bem.info/methodology/css/#openclosed-principle)
* [The Open/Closed Principle in CSS Guidelines](https://cssguidelin.es/#the-openclosed-principle)
* [The Open/Closed Principle on Wikipedia](https://en.wikipedia.org/wiki/Open/closed_principle)

Formally the open/closed principle reads

> Software entities (classes, modules, functions, etc.) should be **open for
> extension**, but **closed for modification**.
Applied to CSS, this means that CSS rulesets should be treated as immutable.
Once a declaration block for a specific selector has been defined, its style
should not be overridden in a different ruleset using the same selector:

```css
// BAD
.button {
color: blue;
font-size: 1.5em;
padding: 16px;
}

.header .button {
font-size: 1em;
padding: 4px;
}
```

Not only does this example violate the semantics of a BEM block, it also
**modifies** the meaning of `.button` depending on the context. Instead, this
could be expressed using a block modifier or a completely separate class which
is kept DRY using mixins:

```css
// GOOD
.button {
color: blue;
font-size: 1.5em;
}

.button--small {
font-size: 1em;
padding: 4px;
}
```

#### Exception: Block Groups

It is a common occurrence that groups of blocks within a container should be
styled differently that single blocks in order to indicate their association.
But using pure BEM mixes of blocks and group modifiers would sometimes require
a large number of modifiers to be applied to multiple elements to achieve that,
e.g.:

```css
.kuiCard { /* ... */ }

.kuiCard__description { /* ... */ }

.kuiCardGroup { /* ... */ }

.kuiCardGroup__card { /* ... */ }

.kuiCardGroup__cardDescription { /* ... */ }
```

```html
<div class="kuiCardGroup">
<div class="kuiCard kuiCardGroup__card">
<div class="kuiCard__description kuiCardGroup__cardDescription">
Card Description
</div>
</div>
</div>
```

To avoid the combinatorial explosion of classes with such groupings and its
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its reads weirdly here. Should it be their? Is it referring a block?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 it is referring to groupings, so their would be correct

modifiers, it is permissible to nest a block's selector under another block's
selector if:

* The relationship is a if a very narrow `element` to `elementGroup` kind that
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoa, what happened with this sentence?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, fixed

is apparent from the block names.
* The rulesets are colocated in the same component directory.

```css
.kuiCard { /* ... */ }

.kuiCard__description { /* ... */ }

.kuiCardGroup { /* ... */ }

.kuiCardGroup .kuiCard { /* ... */ }

.kuiCardGroup .kuiCard__description { /* ... */ }
```

#### Exception: Normalization/Reset

When working with the browser default styles or third-party stylesheets the
selectors are sometimes overly broad so that they have to be re-used in order
to integrate the styles into the project.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by re-using a selector so that the styles can be integrated into the project? Are there examples of this in the codebase?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of more generic selectors like a or input[type="text"], which are often styled by third-party libraries without care. Or take a widget library like ui-select, which does not allow the developer to specify his own classes. You're forced to modify existing classes provided by those libraries.

Copy link
Contributor

@cjcenizal cjcenizal Jun 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that makes a lot of sense. Could we reword this part? I usually think of this in terms of overriding selectors which we don't control. My suggestion:

We can't control the selectors introduced by third-party stylesheets and
these selectors may not adhere to our styleguide, e.g. `a` or `input[type="text"]`.
In these cases, we're forced to duplicate these selectors within our own
stylesheets and override those styles to control their look and feel.

When this happens, it's important to add comments which make it clear why these
selectors exist and which third-party dependencies they override. We should
also define these selectors in files which refer back to the dependency,
e.g. a file named `ui_select_overrides.less` will contain styles overriding those
introduced by the `ui-select` dependency.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absolutely, thanks for the improvement!