Problem/Motivation

We've tried to use #1190252: [573] Use csslint as a weapon to beat the crappy CSS out of Drupal core to fix css in core. This community initiative did some great work but never got to the stage where our css was totally compliant. We now have problems because we're trying to turn on csslint as part of automatic coding standard checking. However the frontend community has coalesced around a different tool - https://stylelint.io/ - for stats on usage of common CSS linting tools see http://www.npmtrends.com/csslint-vs-csscomb-vs-stylelint

This is @droplet's idea - see #1190252-91: [573] Use csslint as a weapon to beat the crappy CSS out of Drupal core. For those new to stylelint here is pafe with some helpful articles: https://stylelint.io/user-guide/articles/

Proposed resolution

  1. Add a basic .stylelintrc with a few simply agree rules - 2 space indentation for example that are whitespace only
  2. Run https://github.com/morishitter/stylefmt#basic on the css so these simple rules apply (Yay there is a fixer) the fixer doesn't work with Drupal standards :( (yet)
  3. Follow up: #2866840: Use stylelint as opposed to csslint in DrupalCI
  4. Follow up: #2876298: Contrib, csslint and stylelint (discuss whether to remove .csslint from core)
  5. Create follow ups to enable non-whitespace rules that enforce the Drupal CSS standard - see child issues in side bar
  6. (non-blocking) Create a follow ups to discuss other stylelint rules

Brief instructions on running stylelint - you'll need yarn...

All the commands below take place in DRUPAL_ROOT/core
To install stylelint - apply this patch and run:

yarn install

This will install Drupal 8's javascript dependencies of which stylelint is one.

To run it on all core css files. Run the following command from DRUPAL_ROOT/core

yarn run lint:css

This issue is scoped to just set up stylelint. Currently it is not replacing csslint. Once this issue has been committed we will transition DrupalCI to use stylelint for core css linting and then we'll come up with a plan for contrib and determine whether we will continue to support csslint. See the followups!

Signoffs from relevant maintainers

  • JohnAlbin (CSS) -- no response; contacted May 1
  • joëlpittet (theme system) -- Signed off in #56
  • Cottser (Frontend framework) -- Signed off in #47

Remaining tasks

User interface changes

None

API changes

None

Files: 
CommentFileSizeAuthor
#74 2865971-74.patch48.65 KBalexpott
#74 71-74-interdiff.txt507 bytesalexpott
#71 2865971-71.patch48.56 KBalexpott
#65 2865971-65.patch88.23 KBalexpott
#65 63-65-interdiff.txt486 bytesalexpott
#63 2865971-63.patch88.04 KBalexpott
#62 2865971-62.patch87.75 KBalexpott
#61 2865971-61.patch42.55 KBalexpott
#61 57-61-interdiff.txt826 bytesalexpott
#57 2865971-57.patch42.37 KBalexpott
#57 46-57-interdiff.txt1.36 KBalexpott
#46 2865971-46.patch43.73 KBalexpott
#46 44-46-interdiff.txt458 bytesalexpott
#44 outside-in-interdiff.txt1.92 KBalexpott
#44 2865971-44.patch43.73 KBalexpott
#41 2865971-41.patch43.73 KBalexpott
#41 29-41-interdiff.txt975 bytesalexpott
#29 2865971-29.patch43.84 KBalexpott
#29 25-29-interidff.txt196 bytesalexpott
#25 2865971-25.patch43.65 KBalexpott
#25 20-25-interidff.txt427 bytesalexpott
#22 interdiff.patch70.8 KBdroplet
#22 2865971-22.patch103.94 KBdroplet
#21 git-diff-whitespace.txt18.66 KBalexpott
#20 2865971-20.patch43.68 KBalexpott
#20 19-20-interidff.txt7.9 KBalexpott
#19 2865971-19.patch39.24 KBalexpott
#19 8-19-interdiff.txt25 KBalexpott
#5 2865971-5.patch16.43 KBalexpott
#5 2-5-interdiff.txt3.14 KBalexpott
#2 2865971-2.patch13.8 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
FileSize
13.8 KB

Here's a fix roll of a patch with an extremely minimal ruleset. The only thing that is fixed here is indentation. Unfortunately the fixer doesn't respect all the rules supported by stylelint. However the fact a fixer exists and will improve over time is great.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
alexpott’s picture

Some pretty easy rules to enable and also to show off the fine grained control we have to tune rules inside files.

alexpott’s picture

We can do better :)

alexpott’s picture

Issue summary: View changes
FileSize
14.78 KB
1.68 KB

Oh and we should totally use our package.json for this - since then we get plugins like https://github.com/Slamdunk/stylelint-no-browser-hacks

If you apply this patch remember to add core/node_modules to your .gitignore. There is or should be an issue to add a .gitignore to the core directory so that no one needs to remember to do this.

alexpott’s picture

Enabled the browser hack detection... Interestingly the only browser hack added is in test css!

modules/system/tests/modules/layout_test/css/layout-test-2col.css
  6:1  ✖  Unexpected selector hack "* html .layout-example-2col .region-left"    plugin/no-browser-hacks
 14:1  ✖  Unexpected selector hack "* html .layout-example-2col .region-right"   plugin/no-browser-hacks
iskin’s picture

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

cosmicdreams’s picture

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.

martin107’s picture

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’s picture

@cosmicdreams we're hardly testing for anything at the moment. We need to flesh out the ruleset. Stylelint comes with many many rules but the current config in the patch doesn't enable hardly anything so you're not comparing apples with apples.

I think we should get a ruleset that passes for now and then add/configure each rule that core does not pass in its own issue so that we fix the each thing one by one. Scope is easy to manage that way and it is working for PHP. The approach of csslint to just add a ruleset that we don't pass and some how fix each css file has not worked.

alexpott’s picture

@martin107 stylelint provides the ability to disable specific rules actually in the CSS files - quickedit CSS can opt out of this rule but benefit from ll the rest.

martin107’s picture

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

alexpott’s picture

alexpott’s picture

cosmicdreams’s picture

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

alexpott’s picture

Well I'm trying to get a whitespace only change ready that accords to the rules in https://www.drupal.org/docs/develop/standards/css/css-coding-standards and then post a list of things we need to do in sub-issues once we land this.

alexpott’s picture

So reading some more. We should extend from https://github.com/stylelint/stylelint-config-standard. This is the basic standard that heaps of others extend from.

The patch attached does exactly that and then NULLs a few rules that either we don't comply will and would make the patch massive or need further discussion because they are not covered by https://www.drupal.org/docs/develop/standards/css/css-coding-standards.

I think if we can get agreement on the current patch we could commit this and then file a followup to discuss/fix each custom rule in the added core/.stylelintrc.json file. We should also open a follow plan to discuss all the other rules that stylelint can enforce that are not covered in the https://github.com/stylelint/stylelint-config-standard or our core/.stylelintrc.json.

alexpott’s picture

Found another whitespace change that is pretty simple just to use the default from stylelint-config-standard for (and it is part of our standards).

alexpott’s picture

FileSize
18.66 KB

Here's a git diff generated with git diff -w since this is way easier to review.

droplet’s picture

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

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

alexpott’s picture

@droplet

+++ b/core/.stylelintrc.json
@@ -6,10 +6,12 @@
-    "color-hex-case": null,
-    "color-hex-length": null,
     "comment-empty-line-before": null,
-    "declaration-block-no-duplicate-properties": null,
+    "declaration-block-no-duplicate-properties": [true, {
+      "ignore": [
+        "consecutive-duplicates-with-different-values"
+      ]
+    }],
     "declaration-block-no-redundant-longhand-properties": null,
     "declaration-block-no-shorthand-property-overrides": null,
     "declaration-block-trailing-semicolon": null,
@@ -18,8 +20,6 @@

@@ -18,8 +20,6 @@
     "function-linear-gradient-no-nonstandard-direction": null,
     "function-name-case": null,
     "function-whitespace-after": null,
-    "indentation": 2,
-    "length-zero-no-unit": null,

It is sad that you made these changes because they introduce non whitespace change. Having only whitespace change makes this patch easy to review. We can file follow up issues to correct them. Trying to do everything at once is what makes these issue never land. And why core never got to the point of implementing the csslint rules :( Instead you could have reviewed the changes and marked this rtbc if you agreed that nothing was against the drupal standard. Which is was because all the rules were set to null which means don't apply them.

alexpott’s picture

Issue summary: View changes

Updated issue summary to provide more detail about the plan here as per #23.

alexpott’s picture

@droplet nice spot on not needing override the indentation.

The last submitted patch, 22: 2865971-22.patch, failed testing.

droplet’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Like eslint we should extend core's standard in the root. This will make life simpler for DrupalCI testing contrib.

droplet’s picture

Will #29 make it unable to add extra rules? (same as the JS problem? #2815077: Adopt airbnb javascript style guide v13 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..)

droplet’s picture

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...

xjm’s picture

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.

xjm’s picture

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.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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?

droplet’s picture

Status: Needs work » Reviewed & tested by the community

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

xjm’s picture

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.

xjm’s picture

Issue tags: +Needs change record

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

martin107’s picture

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

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

martin107’s picture

Issue tags: -Needs change record
Cottser’s picture

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

alexpott’s picture

@xjm is right we shouldn't be making non-whitespace change in this patch. We can leave the comment in and adhere to the stylelint rules by just making the */ on the next line. Fixed another place where we are removing commented out css.

Mile23’s picture

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 41: 2865971-41.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
43.73 KB
1.92 KB

Rerolled due to #2862625: Rename offcanvas to two words in code and comments. .

 2865971  alex: ~/dev/sites/drupal8alt.dev/core > npm run lint:css

> Drupal@ lint:css /Volumes/devdisk/dev/sites/drupal8alt.dev/core
> stylelint '**/*.css' || exit 0

 2865971  alex: ~/dev/sites/drupal8alt.dev/core >

No errors.

To review the patch git diff --color-words --word-diff-regex=. it really useful.

idebr’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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"

alexpott’s picture

Status: Needs work » Needs review
FileSize
458 bytes
43.73 KB

@idebr thanks for testing this on Windows!

I can confirm that your suggestion works great on OSX too.

Putting back to needs review because we have still have "Needs subsystem maintainer review, Needs framework manager review". @Cottser is both of these and has commented favourably in #40.

Cottser’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs framework manager review

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.

droplet’s picture

Just simply drop the quote will work :P

idebr’s picture

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.

alexpott’s picture

On OSX at least
"lint:css": "stylelint **/*.css || exit 0"
Doesn't actually find anything... if you apply the patch make the change suggested - ie remove the quotes and then add a space some in a css file and run linting it finds nothing!
That's why I put the quotes there originally :)

joelpittet’s picture

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!

droplet’s picture

@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’s picture

@joelpittet re #51 there is no /.stylelintrc.json in the patch - there is only core/.stylelintrc.json which defines what core adheres to.

joelpittet’s picture

@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! "

alexpott’s picture

Ah yes @joelpittet you are right - actually this is for contrib ala the eslint config. I'd prefer to keep it in there. There was a meeting about javascript linting at Baltimore and we agreed to do it this way for JS for the time being. Custom work can alway implement whatever standards they like inside their own theme or module.

As an aside, personally I don't think that we should have these in the root directory either. In my opinion we should publish the core JS and CSS standards on npm and allow contrib and custom projects to include and adapt as they see fit. Why? Because this is the right way for dependencies. However we have a lot of work to get all dependencies round the right way - look at the fun we're having with drush! #2874827: Drush 8.x doesn't install Drupal 8.4.x and Drush master doesn't install Drupal 8.3.x

BUT thew agreed idea at the moment is that contrib and custom can run things from the Drupal root directly like eslint modules/my_module and get the up-to-date core standards. And DrupalCI will follow suit. It is also agreed that if the module / theme supplies it's own config then DrupalCI will use that. (There are problems with this because DrupalCI is not doing npm install when it starts testing but hey one thing at a time - so for custom support you can't extend from default rulesets or someone elses standards but I think we need to do one thing at a time).

For me this issue takes closer to getting core's CSS adhering to our standards and starts to open up the possibilities for contrib and custom but we should stop doing this because the solution is not perfect for them yet.

joelpittet’s picture

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.

alexpott’s picture

Actually @joelpittet you've convinced me! We should always do the smallest possible change.

We should just add the plumbing in this issue and leave contrib and DrupalCi's testing of CSS standards alone. And then we can get DrupalCI to support core and contrib linting via this as next steps. Which would be for DrupalCI to swap to stylelint where it is available - and so core would get covered and then we can work out a plan for contrib.

lauriii’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update, +Needs followup

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!

martin107’s picture

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?

alexpott’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update, -Needs followup

Created #2876298: Contrib, csslint and stylelint (discuss whether to remove .csslint from core) to discuss the issues for contrib. And #2866840: Use stylelint as opposed to csslint in DrupalCI already exists to move DrupalCI to using stylelint for core CSS linting.

Also updated the change record for the more limited ambition of this patch.

Everything from #58 has been addressed as far as I can see.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
826 bytes
42.55 KB

Let's make life simple for DrupalCI and get everything in place so they only need to make changes in DrupalCI.

alexpott’s picture

alexpott’s picture

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

One more crept in in #2843083: Select entity type / bundle in workflow settings

yarn run lint:css
yarn run v0.23.4
$ stylelint "**/*.css" || exit 0

themes/seven/css/components/dropbutton.component.css
 206:48    Expected single space before "{"   block-opening-brace-space-before

  Done in 4.65s.
xjm’s picture

Issue summary: View changes
xjm’s picture

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?

alexpott’s picture

@xjm removal of csslint will happen in the followups #2866840: Use stylelint as opposed to csslint in DrupalCI and #2876298: Contrib, csslint and stylelint (discuss whether to remove .csslint from core) - so core will use stylelint and then we'll work out the best way to support contrib. We have to solve exactly the same issue post #2815077: Adopt airbnb javascript style guide v13 as new baseline javascript coding standards for Drupal 8 core and contrib which left the root .eslintrc too. These followups are listed in the issue summary and the decision to do it this way was in response to sub-system maintainer suggestions ie #56.

xjm’s picture

+++ 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.

xjm’s picture

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

alexpott’s picture

On @xjm's recommendation broke out the whitespace fixes into a separate patch (#2878548: Fix CSS whitespace issues) Here's a patch containing just the stuff for stylelint.

In order to get a lint that passes you have to apply the other patch too. Leaving at RTBC because the patch is the same patch as #65 just with everything whitespace related removed.

Also no interdiff because it would be bigger than the patch :)

alexpott’s picture

Ticking the credit boxes for @xjm, @martin107, @joelpittet, @idebr and @Cottser for their reviews of and input into this patch.

droplet’s picture

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

should we keep it same as Babel config?

alexpott’s picture

@droplet good idea!

droplet’s picture

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

cilefen’s picture

Ticking a box.

  • cilefen committed 9a0e9a6 on 8.4.x
    Issue #2865971 by alexpott, droplet, xjm, martin107, joelpittet, Cottser...
cilefen’s picture

Status: Reviewed & tested by the community » Fixed

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

I published the CR.

drpal’s picture

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.

cilefen’s picture

droplet’s picture

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’s picture

@droplet I promise that going per-rule will make it quick. It means each issue will review the rule and implementation. Some rules might require debate and doing that in a group issue will cause the entire group to be delayed.

droplet’s picture

@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!

alexpott’s picture

@droplet so the process of scoping each patch into a different task is borne from experience. For instance, when @xjm asked for the CSS changes to be split out from this task it was to make it easier to review and single thing. And even though the CSS changes in #2878548: Fix CSS whitespace issues were only whitespace - ideally it would have been rule by rule too, but, you have to start somewhere. And that patch could be reviewed simply with an ignore whitespace diff command. What's written on https://www.drupal.org/core/scope is not to make changes go slower or to not be optimised - it is because it is the optimised workflow when you have many contributors, many tasks, and many reviewers.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.