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

CSS/SCSS indenting #3012

Closed
holloway opened this issue Oct 11, 2017 · 20 comments
Closed

CSS/SCSS indenting #3012

holloway opened this issue Oct 11, 2017 · 20 comments
Labels
keep-unlocked lang:css/scss/less Issues affecting CSS, Less or SCSS status:needs discussion Issues needing discussion and a decision to be made before action can be taken type:enhancement A potential new feature to be added, or an improvement to how we print something

Comments

@holloway
Copy link

holloway commented Oct 11, 2017

This is a suggestion for a change option in formatting SCSS/CSS.

When reading JavaScript we all know how beneficial indentation can be to express nesting blocks, but in CSS there are also defacto and popular ways of expressing nesting that I think Prettier could automate.

BEM codifies some defacto standards in naming conventions for CSS, in which classes are named as,

.block { /*props*/ } 

  .block__child1 { /*props*/ } 

    .block__child1--modifier1 { /*props*/ } 

    .block__child1--modifier2 { /*props*/ } 

  .block__child2 { /*props*/ } 

    .block__child2--modifier { /*props*/ } 

Here nesting is expressed in a naming convention but also whitespace, and when I'm reading CSS I often find that classNames have hierarchy expressed in naming conventions (with SCSS of course programmers can also nest selectors and use & but that's a separate issue).

Would you be open to accepting a patch to indent CSS/SCSS blocks based on the previous selector?

I think the formatting rule could be that if the previous selector is a prefix of the current selector then we consider that to be nesting and the block is nested a level. Any name that doesn't match would be formatted as Prettier currently does it (pushed left-most). Selectors would need to be contiguous to be indented. Of course there might be some details to work out but I just want to know if there's interest in this type of formatting for CSS/SCSS?

Just to be more specific...

Input:

.reposition {
	display: flex;
	align-items: center;
}

.reposition button[disabled] {
	opacity: 0.25;
}

.maps {
	display: block;
	overflow: auto;
}

.maps--disabled {
	opacity: 0.5;
	pointer-events: none;
}

Current Output:

.reposition {
  display: flex;
  align-items: center;
}

.reposition button[disabled] {
  opacity: 0.25;
}

.maps {
  display: block;
  overflow: auto;
}

.maps--disabled {
  opacity: 0.5;
  pointer-events: none;
}

Expected behavior:

Output:

.reposition {
  display: flex;
  align-items: center;
}

  .reposition button[disabled] {
    opacity: 0.25;
  }


.maps {
  display: block;
  overflow: auto;
}

  .maps--disabled {
    opacity: 0.5;
    pointer-events: none;
  }
@suchipi
Copy link
Member

suchipi commented Oct 12, 2017

How popular is this style? I've never run into it (but the apps I've worked on used sass or stylus rather than BEM or similar methodologies, so I think it was more common to express stuff via nesting).

I mostly do component-based styling these days, so I don't have a good idea of what common CSS formatting in the community looks like.

@ikatyang ikatyang added lang:css/scss/less Issues affecting CSS, Less or SCSS status:needs discussion Issues needing discussion and a decision to be made before action can be taken labels Oct 12, 2017
@holloway
Copy link
Author

holloway commented Oct 12, 2017

Yep it's fairly popular in large-scale CSS frameworks, and in books about CSS. Here are some examples:

Finally, there doesn't seem to be anyone against more indentation that I've found (some people conflate formatting with the bad practice of making overly specific CSS selectors but of course those are unrelated issues), but maintaining the indentation manually is a hassle -- and solving manual formatting is the whole point of Prettier!

So where to from here?

@lydell
Copy link
Member

lydell commented Oct 12, 2017

Some random things to discuss:

1. At work, we have one "BEM root" per file. For example, a file could be called product-card.scss and all selectors in that file start with .product-card (such as .product-card__image). This means that everything except the first rule of every SCSS file we have will be indented. So for our purposes, this change would not make things better (but not very much worse either). I thought BEM recommended this approach of one "BEM root" per file, though? To use the file system as a "collision detector" for class names.

2. Actually, our classes look more like .ProductCard and .ProductCard-image, because we follow the SUIT CSS naming convention. Have you thought about what naming conventions to support?

3. I think Facebook uses Prettier to format their CSS these days, so I'm pinging @vjeux. Would this break havoc on your CSS?

4. Should this be behind an option?

5. It might be worth doing a PR now to see what things would look like, but with the obvious risk of it never being merged. Your call.

@vjeux
Copy link
Contributor

vjeux commented Oct 12, 2017

  1. it absolutely would! :(

@duailibe
Copy link
Member

  1. I don't think an option would be bad for this particular case

@holloway
Copy link
Author

holloway commented Oct 12, 2017

  1. I'm not sure that this would need to support or prefer any particular naming conventions for separator characters (- vs _ etc.). In your scenario of .ProductCard and .ProductCard-image any nesting would occur because .ProductCard-image begins with the string .ProductCard, and of course only when .ProductCard is directly followed by .ProductCard-image (they must be adjacent). So .ProductCardImage without a - would also be nested.
  1. With @vjeux's feedback I'll assume that an option is the only way this idea could go forward. I'm not sure what to call it, but perhaps nestAdjacentStyles?
  2. I'm happy to start on a PR for this in a day or two unless there's a clearer sign that this isn't wanted, so sleep on it and please let me know, and yes I understand the code might be discarded.

Thanks for considering this!

@ArmorDarks
Copy link

I don't think that relying on naming convention is a good idea here, because BEM is BEM, but some projects using full BEM, some are camelCase version of BEM and so on, most people using Harry Roberts version. In fact, original BEM naming is less popular nowadays that its successors.

Thus, we will have hard time figuring out where is block, element and modifier. Or at least it will require ability to configure what will be considered a block, element and modified.

Not to mention that some methodologies are extended. For instance, Harry Roberts suggested block@modifier for indicating breakpoint. And it is widely used.

As an opposite solution, Prettier can ensure that nesting is consistent.

For instance, it will fix this:

.block {
    /* styles */
}

         .block__foo {
                   /* styles */
             }

To this (since it can not be nested that deep anyway)

.block {
    /* styles */
}

    .block__foo {
        /* styles */
    }

But will not touch

.block {
    /* styles */
}

.block__foo {
    /* styles */
}

because nesting isn't compromised.


Also, as a side note, modifiers tend to be putted on same level as block or element, since they aren't their sub-part in DOM, they just modifies them:

.block {
    /* styles */
}

    .block__foo {
        /* styles */
    }

    .block__foo--modifier {
        /* styles */
    }

@azz
Copy link
Member

azz commented Oct 13, 2017

I personally find this style harder to scan, and I have also never encountered it until this issue...

@ikatyang ikatyang added the type:enhancement A potential new feature to be added, or an improvement to how we print something label Oct 15, 2017
@holloway
Copy link
Author

  1. @lydell ... there's a WIP PR over here now ➜ CSS Hierarchy Indent option (off by default) #3038

    I think it's a fairly minor change that shouldn't disrupt the normal codepaths, and it (arguably!) improves readability by grouping CSS Rules.

  1. @ArmorDarks I agree that we shouldn't favour any particular naming convention and the WIP adheres to this principal. This does mean that what you refer to as 'modifiers' aren't a special case, and they are indented accordingly (with the current approach, anyway).

@ryami333
Copy link

ryami333 commented Oct 16, 2017

Being behind a flag, I don't feel there's any harm in adding this feature. I'd like to address whether there's a want for this throughout the dev community though (rather than just our individual preferences and opinions). Sitepoint undertake a CSS survey every year, and here's a couple of excerpts from this year's results:

screen shot 2017-10-13 at 12 15 57 pm

So, the vast majority of people see value in indentation (either pre-processor style, or manually). Of the remaining ~quarter of people, I wonder how many of them don't indent because it's cumbersome to do so manually. I wouldn't be surprised if more people preferred indentation if their tooling did if for them.

There's also a few of us mentioning their alternative bespoke naming schemas, so I wanted to know whether there's actually any weight in any of them:

screen shot 2017-10-13 at 12 18 10 pm

screen shot 2017-10-13 at 12 21 52 pm

BEM's the clear winner when it comes to strict methodologies, but people are also using camelCased and snake_cased alternatives too, all of which would be compatible with this PR, as far as I can tell. Those not using a methodology, well, we probably can't help those people one way or the other 😂

So, yes I do think the community wants this. Most of us are indenting, and most of us are using compatible naming schemas.

@azz
Copy link
Member

azz commented Oct 16, 2017

Most of us are indenting

I think that might be misinterpreting the results. 24% are indenting. 44% are using a pre-processor. We don't know if they would indent the code if they weren't using a pre-processor (I don't, for example).

@ryami333
Copy link

ryami333 commented Oct 16, 2017

You nest flat against the left hand side? Eg.

parent {
child { /* styles */ }
}

I've never seen that before.

EDIT: Anyway, I agree you could interpret it that way too, but by putting this behind a flag - we're not stepping on your toes. No matter how you interpret the pre-processor block in those results, there's still a large portion of people with an appetite for this feature.

@azz
Copy link
Member

azz commented Oct 16, 2017

With pre-processor:

.block {
  .modifier {
  }
}

Without:

.block {
}

.block .modifier {
}

@holloway
Copy link
Author

holloway commented Oct 16, 2017

Thanks, so with Pre-processor users (which includes me, most of the time!) Prettier is able to help and provide automated indentation.

So let's try looking at the Sitepoint stats another way... 44% use nesting in Pre-processors, but really the portion of the stats that we're talking about are the others: those who use plain CSS, and those who don't use Pre-proccesor nesting (maybe to make searching for selectors easier, from what I've heard), and that leaves the stats 28% flat vs 24% nested for those developers with an opinion. As strange as it sounds that's more users than Pre-processors alone. Just comparing those two figures makes it an equivalent ratio of 53% flat vs 46% nested in the other camp.

Formatting issues and white-space are subtle things to discuss and articulate, but I think we can agree that everyone indents Sass for readability (yuss!), and 46% is about half the community who indent while using the others who could benefit from Prettier standardisation.

Anyway, I don't know how to progress this further without repeating myself because I'm all out of data and examples 😀 Ultimately now it's your choice about whether this is a useful feature or not, and the PR has the beginnings of an implementation, so I'll keep quiet until I get some feedback.

Thanks for considering this.

@csswizardry
Copy link

Quickly popping my head in to say:

  1. I strongly advocate this nesting-as-documentation method (might even go as far as to say that I coined it).
  2. Sass-style nesting is largely considered an anti-pattern these days, but that’s a whole other discussion.
  3. Absolutely put this behind an option. It’s gaining traction but is far from the norm (yet… I’m patiently waiting).

Thanks!
H

@keithjgrant
Copy link

keithjgrant commented Oct 19, 2017

I agree that indenting to simulate nesting is a moderately common convention (about 25% of devs, per the poll above—which jives with my sense from the community). It might be worthwhile as an option, but I don’t think it should be the default/only behavior.

@Kelly-Bramford
Copy link

We use this style at work to see related CSS together. Initially it was a bit strange to read- like looking at JSX (from Facebook React) but now it's great, and it helps developers communicate relationships between CSS

@katie-day
Copy link

One advantage that indenting has, and that aligning flush with the left doesn't, is it helps keep class names consistent and will reduce unintentional errors. The indenting would clearly show if you'd made a spelling mistake or if you'd named a child differently to its parent. You'd be expecting an indentation whereas it would be flush left, immediately you could visually identify the error. It's like a nesting error in JavaScript that you can fix there instead of at run-time.

CSS codebases have naming conventions so helping expose them makes sense.

@iandevlin
Copy link

I must admit that this is the first I have ever heard of indenting CSS in this way, and I am not a fan right now anyway. I have nothing against others liking such things of course (and perhaps I become one of them at a future date), but it should be optional.

@alexander-akait
Copy link
Member

/cc @duailibe @j-f1 looks like we can close issue, i think better create stylelint rule for this with autofixing

@suchipi suchipi closed this as completed May 24, 2018
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Aug 22, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 22, 2018
@j-f1 j-f1 added keep-unlocked and removed locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. labels Sep 3, 2018
@prettier prettier unlocked this conversation Sep 3, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
keep-unlocked lang:css/scss/less Issues affecting CSS, Less or SCSS status:needs discussion Issues needing discussion and a decision to be made before action can be taken type:enhancement A potential new feature to be added, or an improvement to how we print something
Projects
None yet
Development

No branches or pull requests