Use stylelint as opposed to csslint in core

Facebook uses Stylelint too and their article could be helpful:
https://code.facebook.com/posts/879890885467584/improving-css-quality-at...

2 is a lot better than 533 but is this being robust enough to catch all the things that the previous test did. That's such a dramatic difference that I wonder if this is accurate.

2 is a lot better than 533

one of the rules dictating that id selectors should be removed in favor of class selectors accounted for the bulk of the errors

The outside in module deliberately ignoring the rule for design reasons...

Actually, even though this is technically a break with CSS coding standards it is *very* intentional that everything is scoped to the ID.

https://www.drupal.org/node/2858879#comment-11995483

I'm just providing context - I don't want to spark a bikeshed but if we introduce an equivalent rule then the error count will jump up.

@alexpott - thanks for the update ... that sounds like a good way of solving a thorny issue.

@alexpott: Oh cool looks like we've got a clear path forward. Is there a specific part of this I can help with?

I agreed there's some ruleset we could enable later.

@see patch, it's more close to Drupal CSS Standard.

OK. Breakdown and commit fast, it's my style on own daily work.

Will #29 make it unable to add extra rules? (same as the JS problem? #2815077: Adopt airbnb javascript style guide v14.1 as new baseline javascript coding standards for Drupal 8 core and contrib. Yeah, we do not redefine the CSS Standard but suddently more Linting errors..)

Anyway, I submitted an issue about the empty line to stylefmt: https://github.com/morishitter/stylefmt/issues/287
I've patched my local version and generated patch #22. It seems pretty well so far. I will wait for stylefmt maintainers feedback and see what I can do this weekend (after more testing) :)

On another side, we could consider updating our CSS standard. "empty line" standard in Drupal isn't so popularity in the wild I think...

Change record maybe?

Also I asked myself whether this should be a TWG decision, but I guess the TWG hasn't made a decision on CSSlint or our current rules either. So core leading by example to create the defacto standards does seem a good first step, to carry forward the mobile and HTML5 initiatives' work to something we can support.

Also while I would love to commit this I think it probably should have explicit signoff not only from our frontend framework managers but also from CSS maintainers.

+++ b/core/modules/outside_in/css/outside_in.motion.css
@@ -34,11 +33,3 @@
-
-/* Transition the administration tray.
-#toolbar-administration,
-#toolbar-administration * {
-  -webkit-transition: all .7s ease;
-  -moz-transition: all .7s ease;
-  transition: all .7s ease;
-}*/

These lines are just being removed rather than reformatted; this is either out of scope or a bug?

We won't leave comment out code in source control?

Not going to play status wars, but it's out of scope. If there's a CSS rule for not having commented-out code, then that should be handled in a separate issue. See:
https://www.drupal.org/core/scope#coding-standards

Outside In is an experimental module. Yes, they should have an @todo attached to this rather than just commenting it out; but it's not in scope for this issue to remove it without letting the Outside In maintainers make a decision on it. It should happen in a separate issue. That issue can be a blocker or a followup, but it does not belong here. This patch should only fix the one rule and add the new API.

Also I still do think it should have a CR as well, in addition to the needed signoffs and the scope question. Thanks!

I have created a first draft CR. please feel free to adjust as you see fit.

https://www.drupal.org/node/2868114

I am in favour of this change. Happy to see this area being worked on :)

It would be super-duper neeto if we could output checkstyle XML since that's what the testbot consumes easily. (And anyone building their own CI for Drupal.)

I found this library to do that: https://github.com/davidtheclark/stylelint-checkstyle-formatter I'm not sure if it meets core's needs, being an npm newb. Please give a look.

+++ b/core/package.json
@@ -6,14 +6,18 @@
+    "lint:css": "stylelint '**/*.css' || exit 0"

Running this command on a Windows machine returns

> stylelint '**/*.css' || exit 0

Error: '**/*.css' does not match any files

A quick Google search returns this http://stackoverflow.com/questions/38814200/files-glob-patterns-specifie...

The following code works correctly: "lint:css": "stylelint \"**/*.css\" || exit 0"

I spent a bit of time reviewing the changes (the command @alexpott posted in #44 is indeed useful) and tested the linting process. Back to RTBC and removing the framework manager review tag.

Just simply drop the quote will work :P

Just simply drop the quote will work :P

I can confirm "lint:css": "stylelint **/*.css || exit 0" works just as well on Windows; only using single quotes returns in an error.

I have introduced a variant of this patch for my project team and it has been unanimously well received, so i am looking forward to having this tool available in Drupal core as well.

Big +1 to this issue!

1. Wondering if we should exclude /.stylelintrc.json similar to why we don't have .gitignore anymore to allow developers to add their own?
2. Also we can probably go IE9 with "ie >8", since we don't support IE8 in D8?

I've not used this before and installed it globally first then ran into this "Error: Could not find "stylelint-config-standard". Do you need a `configBasedir`?"
There's an issue working through that.
https://github.com/stylelint/stylelint/issues/1973

Then just used core's packages with the command in #44 and things worked great! Removed some of the changes to see how it picked up, which was sweet!

@joelpittet,

Run npm install or yarn in CORE first

+++ b/core/package.json
@@ -6,14 +6,18 @@
+    "lint:css": "stylelint \"**/*.css\" || exit 0"

we can point it to node ./node_modules/stylelint/bin/stylelint.js. ESLint will do it also

@alexpott yes there is one in the web root:

+++ /dev/null
diff --git a/.stylelintrc.json b/.stylelintrc.json
new file mode 100644
index 0000000..2df6b94
--- /dev/null
+++ b/.stylelintrc.json
@@ -0,0 +1,3 @@
+{
+  "extends": "./core/.stylelintrc.json"
+}
diff --git a/core/.stylelintrc.json b/core/.stylelintrc.json

Maybe it was added by mistake, but regardless I think it should be removed ala .gitignore.

@droplet, yeah, like I said "Then just used core's packages with the command in #44 and things worked great! "

I'm still thinking it would be easier to keep this for and in core/ only, and help core build up its standards and maybe add something like this later to help contrib or contrib can do what you said and use an NPM library. It sounds like we are taking on a bit more responsibility than we should if we do this for contrib too, but that's just my thoughts.

This is still RTBC from my review and I'm removing the subsystem reviewer because I feel subsystem bleeds into this, or at least most theme system API peeps are closer to the front-end, plus CSS only has @JohnAlbin, so helping him hopefully here.

Since the proposed solution seems to have shifted, it would be nice if we'd get an update to the proposed solution to the issue summary. Can we also create a follow-up for removing csslint from the core?

Please update necessary bits to the change record as well. We should also explain in the change record what does this mean for contrib and site owners including if there are some actions they should be taking because of this change.

Thank you everyone for working on this!

Please update necessary bits to the change record as well. We should also explain in the change record what does this mean for contrib and site owners including if there are some actions they should be taking because of this change.

I am happy to update the change record. Can I just pick your brains for a moment, Just so I can understand the line you are taking

Is you perspective that contrib should start moving to stylint.io?
OR
Did you want to signal that core's css will change subtly over time so that some themes and some javascript selectors, may have to be updated.

For site owners, should they be prepared for a cascade of theme and contrib updates .. or did you have a different line of reasoning?

That seems reasonable to add the DrupalCI stuff here. Back to RTBC

I reviewed this locally with git diff --color-words -w and confirmed that, aside from the added core/.stylelintrc.json, the only non-whitespace changes are in:

• core/package.json
• core/themes/seven/css/components/dialog.css (a removed commented-out border; should we see why that's there?
• core/yarn.lock (which apparently already existed?)

I also noticed that the patch doesn't seem to remove .csslintrc?

+++ b/core/themes/seven/css/components/dialog.css
@@ -60,7 +60,6 @@
 .ui-dialog .ui-widget-content.ui-dialog-buttonpane {
   background: #f5f5f2;
-  /*border-top: 1px solid #bfbfbf;*/
   margin: 0;
   padding: 15px 20px;
   border-bottom-left-radius: 5px;

So as discussed above in #36 and #41, I would say that we should not be making non-whitespace changes in this patch. (I hesitated even about the whitespace ones, since it makes the patch a lot harder to review. Ideally we'd fix the whitespace and then add the ruleset...)

Looking at the history of this line, though, it's never been uncommented. It was added as an original part of the file in #2113911: Modal style update, and the color doesn't appear in any removed lines there either. Looking for this color rule elsewhere in core, I'd guess the commented-out rule was inherited from Views UI styling.

@alexpott, sorry, I didn't realize that from the issue titles (which in my defense are not very descriptive).

+++ b/core/.stylelintrc.json
@@ -0,0 +1,43 @@
+      "browsers": [
+        "last 2 versions",
+        "ie >=8"
+      ]

should we keep it same as Babel config?

Thanks. Let's move forward. We always able to do a quick iteration :)

Ticking a box.

Committed 9a0e9a6 and pushed to 8.4.x. Thanks!

I published the CR.

It looks like we didn't specify the exact versions of the new stylelint dependencies. What I think should have happened is yarn add stylelint@7.10.1. Despite the yarn.lock file, we are still opening ourselves up to version mismatch issues by being too loose with the exact dependencies.

The ^ will still automatically update us to a newer minor version which could introduce some issues.

Good idea: #2879950: Lock stylelint to a specific version

We have about 20 child issues out there. It's inefficiency to work it out with manpower to fix each error. Now, we have no auto-fix tool to fix single error per time.

If you looking at #22, most of the changes are SPACE, LETTER CASES changes only. I think Core Development should start to accept and learn to work with a better efficiency way to patch our code wisely. (there're many other challenge brain work for our contributors.)

If we still worrying about the mistakes, we can split into GROUP:
- Fix small changes first
e.g.: Fix SPACE, LETTER CAES, ZERO UNTI
- Then, fixing complicated changes
e.g.: background-images, @media rules

FastPatch -> Commit -> FastPatch -> Commit.

I bet 2 ~ 3 commits will sort out all these CSS style errors.

@alexpott,

That's easy. discussion before implementation. This is the second thing the CORE development should LEARN. We should not always ask code patch FIRST and review LAST. We should do all easy things first, then the difficult parts.

That meant, with a better workflow. Even with 20 child issues, we should mark it as REVIEW (or RTBC) status now. And asking for sub-maintainers feedback or others.

At the worse case, 20 child issues will trigger 20 times reroll of whole d.org patches.

Now, we need 20 people to work on 20 issues, and each issue may require 20min ~ 1 hour on code patch only. And uncountable reroll patch because other new patches create conflicts, the delay of review. (More Important: This is a very boring job)

It's hard to bring new bug into CORE with a CSS style formatting. It's a good chance to relax and learn a better way with patching.

Trying and to optimize workflow with the simple thing. It's a good start!