[policy] CSS coding standards: Vertical spacing between selectors

I disagree. I don't think they are smushed together at all.

Adding this for reference though, since we just discussed this last year: #748798: CSS Coding Standards: Selector Separation.

And for this that don't know where the current standards are, see here: http://drupal.org/node/302199

I don't work on core so maybe I only get a +.05 but I definitely prefer a newline before each. Closing brackets touching stuff on the next line gives me the heebie jeebies. :)

Michelle

-1 We had an in-depth discussion on this already.

The current standards dictate to have a blank line between logical sub-groups of styles:

.book-navigation .page-next {
}
.book-navigation .page-previous {
}

.book-admin-form {
}

CSS doesn't make it particularly easy to keep code both readable and clearly arranged at the same time. We're adding extra blank lines between groups to clarify which styles belong to what, and without enforcing a CSSDoc block comment for every smaller grouped chunk.

The blank line for grouping rule is unimportant for automated testing. In fact, all that automated code reviews should check for is that styles are ending in /^\}$/.

IMHO, this issue won't fix.

There doesn't seem to be much interest in a style change and, as sun mentioned, once you get enough CSS in one place a blank line or two doesn't make much of a difference.

There's 2 for and 2 against before your comment. Looks like a tie to me and could use more input. I won't mess with the status because I'll continue doing it the way I want in contrib so am not bound by this but I do think it's a premature closing.

Michelle

This is a wont fix IMO too. Why is the whole conversation we had about this last year is just being ignored? Does that not count?

What I see here is PHP developers pushing to change a standard that front end developers discussed and agreed upon... for parsing. Sigh. :s

Has nothing to do with parsing for me. I just think it looks bad and is harder to read. I suppose that's parsing... Just human parsing. :)

Michelle

The spaces are for logical groups. This is not something that is cut and dry like a PHP function with a docblock. It needs human interpretation. It's not consistent across core yet, but it will be, because we are working on these issues touching every single CSS file in core right now. It would be great if we could at least get to that point before people want to change it.

I disagree. And IMO it's easier to read the way it is right now.

1) The issue that introduced the current standard seems to have been #748798: CSS Coding Standards: Selector Separation, but not 100% sure. That issue isn't very long, and the committed patch performed different changes throughout core. IIRC, this was discussed back and forth in more detail either somewhere on g.d.o or in some other issue(s).

2) This coding standard mostly, primarily, and almost exclusively affects front-end developers, themers, and designers. Because of that, arguments on the grounds of a technical parser, automated code review tools, or "consistency" on a technical (but not logical) level are of minor importance, compared to the needs as well as common and best practices of aforementioned group and primary audience that is supposed to implement this standard.

3) IIRC, plenty of people objected to the idea of adding an extra blank line between style rules. IIRC, the primary reason was that it makes it significantly harder to understand the total structure in (larger) CSS files. And to put this into context: Due to the same reason, many people were or still are writing all styles within a rule onto one line. Thus, by adding even more useless white-space, those themers would most probably be totally thrown off, and wouldn't follow the standards at all anymore. Which, in turn, inevitably leads to the question for whom these CSS coding standards are, if the primary audience disagrees with them.

Personally speaking, and given aforementioned points, you might have more luck by negating/reversing the proposal to state "no blank lines between CSS rules, and everywhere you think there should be a separation, put a CSSDoc block header in front of it."

Readability

Without or not is personal practice more than Readability.

I don't know if it is the originally idea of CSS code standard or not. The example on that page isn't quite well.

myself prefer this way:

.profile {
  clear: both;
  margin: 1em 0;
}

.profile .user-picture {
  float: right; /* LTR */
  margin: 0 1em 1em 0; /* LTR */
}

.profile h3 {
  border-bottom: 1px solid #ccc;
}

.profile dl {
  margin: 0 0 1.5em 0;
}
.profile dt {
  margin: 0 0 0.2em 0;
  font-weight: bold;
}
.profile dd {
  margin: 0 0 1em 0;
}

.profile / .profile .user-picture / .profile h3 are not relative
.profile dl / .profile dt / .profile dd are related elements / selectors

Coding

But with a blank line, I think it will be easier to copy, select & highlight. (myself always selected one more line carelessly)

This is the standard as I read it right now. The comments are the big part that's missing right now, except for certain files where we really focused in D8. Here's a real example from system.base.css, with one change (a blank line added above the animated throbber comment).

This is very similar to your last example, but the comments for the "group" blocks end with a period. There also still needs to be an opportunity to provide comments like for one the .container-inline block that do not denote a new sub group.

/**
 * Autocomplete.
 *
 * @see autocomplete.js
 */
/* Suggestion list */
#autocomplete {
  border: 1px solid;
  overflow: hidden;
  position: absolute;
  z-index: 100;
}
#autocomplete ul {
  list-style: none;
  list-style-image: none;
  margin: 0;
  padding: 0;
}
#autocomplete li {
  background: #fff;
  color: #000;
  cursor: default;
  white-space: pre;
}

/* Animated throbber */
html.js input.form-autocomplete {
  background-image: url(../../misc/throbber.gif);
  background-position: 100% 2px; /* LTR */
  background-repeat: no-repeat;
}
html.js input.throbbing {
  background-position: 100% -18px; /* LTR */
}

/**
 * Collapsible fieldsets.
 *
 * @see collapse.js
 */
html.js fieldset.collapsed {
  border-bottom-width: 0;
  border-left-width: 0;
  border-right-width: 0;
  height: 1em;
}
html.js fieldset.collapsed .fieldset-wrapper {
  display: none;
}
fieldset.collapsible {
  position: relative;
}
fieldset.collapsible .fieldset-legend {
  display: block;
}

/**
 * Inline items.
 */
.container-inline div,
.container-inline label {
  display: inline;
}
/* Fieldset contents always need to be rendered as block. */
.container-inline .fieldset-wrapper {
  display: block;
}

@Jacine,

If the following case, can we do this:

/**
 * Profile selectors
 */
.profile {
  clear: both;
  margin: 1em 0;
}
.profile .user-picture {
  float: right; /* LTR */
  margin: 0 1em 1em 0; /* LTR */
}
.profile h3 {
  border-bottom: 1px solid #ccc;
}

.profile dl {
  margin: 0 0 1.5em 0;
}
.profile dt {
  margin: 0 0 0.2em 0;
  font-weight: bold;
}
.profile dd {
  margin: 0 0 1em 0;
}

OR

/**
 * Profile selectors
 */
.profile {
  clear: both;
  margin: 1em 0;
}

.profile .user-picture {
  float: right; /* LTR */
  margin: 0 1em 1em 0; /* LTR */
}

.profile h3 {
  border-bottom: 1px solid #ccc;
}

.profile dl {
  margin: 0 0 1.5em 0;
}
.profile dt {
  margin: 0 0 0.2em 0;
  font-weight: bold;
}
.profile dd {
  margin: 0 0 1em 0;
}

It is unnecessary comment in some of our CSS code.

/* Profile definition lists */

Yeah, I think that entire example is bogus, because it's (a) obviously styles for the profile module because of the file name and location (b) most of that CSS has no business in core in the first place. That's why I posted a different example where comments and grouping are justified, so we can try to discuss a real use case/situation.

In parts, we're going to risk to duplicate the discussion in #748810: CSS Coding Standards: Comments

Can we just pick one issue and close the other? I don't care which one stays open at this point.

I think this one is focus on SPACE, not the comment part

#17 without LINK SPACE between each comment section
#18 is difference.

It's not clear when should use a BLANK LINE.

If this maybe-a-blank-line-maybe-not-depending-on-individual-thoughts-on-grouping standard is going to stay in the standards, are there going to be docs on when you should and when you shouldn't? I can imagine this causing lots of confusion and bike-shedding over which bits go together and don't get a blank line and which bits don't. All the other coding standards are black and white, very clear on how it should be. Now we have one that's very wish-washy which may be awesome for designers but it gives devs fits. :P So hopefully it will be very clearly documented.

Michelle

I'd actually be ok with the proposal.

AFAICS, it would replace this current fragment on http://drupal.org/node/302199:

.book-navigation .page-next {
}
.book-navigation .page-previous {
}

.book-admin-form {
}

A blank line should be placed between each group, section, or block of multiple selectors of logically related styles.

This proposal would translate into:

Separation and grouping of selectors

/**
 * @file
 * Styles for Book module.
 */

/**
 * Inline items.
 *
 * @see inline.js
 */
/* If necessary, leading sub-group comment. */
.container-inline div,
.container-inline label {
  display: inline;
}
.container-inline p {
  display: inline;
}

/* Fieldset contents always need to be rendered as block. */
.container-inline .fieldset-wrapper {
  display: block;
}

1. Use a blank line before a comment.
2. Use CSSDoc multi-line syntax/formatting for the leading @file as well as section comments.
3. Group multiple, logically related styles by prepending an inline comment.
4. Don't use blank lines between styles.

See also standards for comments. [« non-functional link right now]

Let's mark it RTBC then.

Please update the issue summary with whatever is RTBC.

Jennifer asked this to be assigned to her.

So, I see that sun proposed a standard and cweagans seconded the motion. But there was all kinds of discussion above from other people, and I don't see any more +1s from the theme maintainers listed in MAINTAINERS.txt or Jacine (one of the markup maintainers; sun is the other)...

So, is there actually consensus on this standard being adopted, and how much change to existing theme/module CSS files would be needed if it were adopted? I'm leaving this at RTBC for now, and IMO it is not yet time to add this to http://drupal.org/node/302199 -- thoughts?

I am setting this back to "needs review" and unassigning until I hear for sure that the appropriate maintainers have weighed in.

The lack of progress here and related issues led to a needless, parallel + duplicated effort of improving our CSS coding standards.

Can we move forward here?

I've updated the section in the DRAFT CSS coding standards to reflect (what I consider to be) the logical merging of this idea into that draft. See http://drupal.org/node/1887862

Blank lines

• In general, do NOT separate each ruleset by a blank line.
• If a ruleset has a proceeding Doxygen-sytyle or single-line-style comment that describes it, place a blank line before the comment.
• If two rulesets have no interleaving blank line, they must be logically related. If they are not logically related to each other, add a blank line and a comment describing the second ruleset.

Does that work for everybody? I completely misunderstood sun's comment in #26, so I had to rewrite it to make sense to me. :-)

This issue has been incorporated into the Draft CSS coding standards at #1891580: [policy] Finally agree on CSS coding standards. So if that issue gets officially approved, this issue is done. Either way, I'd consider this RTBC.

The slightly rephrased version in #35 works for me.

The final documentation chapter would need to look like #26 though, but we can do that while incorporating the change into the handbook page.

Unfortunately, we can't use the example code form #26 because .container-inline div doesn't match our new CSS architecture standards for D8. I have updated the example code in http://drupal.org/node/1887862 to be:

/* A comment describing the ruleset. */
.selector-1,
.selector-2,
.selector-3[type="text"] {
  -webkit-box-sizing: border-box;
  -moz-box-sizing: border-box;
  box-sizing: border-box;
  display: block;
  margin: 0;
  font-family: Times, "Times New Roman", sans-serif;
  color: #333;
  background: #fff;
  background: linear-gradient(#fff, rgba(0, 0, 0, 0.8));
}

/**
 * A longer comment describing this ruleset. Note
 * the blank line before the docblock.
 */
.selector-4,
.selector-5 {
  padding: 10px;
}

/* This logical grouping of rulesets has no interleaving blank lines. */
.profile {
  margin: 1em 0;
}
.profile__picture {
  float: right; /* LTR */
}

I believe that example conveys the same information as was in #26.