Problem/Motivation
I know morten likes to complain about the CSS in Drupal core. And while he does love arguing, its much easier to change the CSS in core if we have useful metrics to show why the CSS is crappy.
Proposed resolution
Stubbornella (Nicole Sullivan) has been a long-time advocate of simple CSS to improve front-end performance. Complex selectors take more time for browsers to match against them. Needlessly-complex selectors lead to crazy cold-war-scenario specificity escalation as each rule tries to out-specify the more general rules.
She has also just released an open-source tool called csslint that we can use to test our current CSS rules.
http://www.stubbornella.org/content/2011/06/15/css-lint-open-sourced/
AND
http://csslint.net/
We've declared which rules we choose to follow inline with our CSS standards in #2222049: Add a .csslintrc file that's in line with our CSS standards
For those wishing to run csslint locally, follow the instructions for the Drupalcore frontend toolkit
Remaining tasks
Module CSS
User interface changes
None
API changes
None
Comment | File | Size | Author |
---|---|---|---|
#28 | test-gruntfile-2222049-do-not-test.patch | 1.42 KB | LewisNyman |
Comments
Comment #1
Stalski CreditAttribution: Stalski commentedSubscribe. Agreed as good starting point.
Comment #2
Maikel CreditAttribution: Maikel commentedSubscribe. Interested in seeing how this will be implemented.
Sidenote: Does this also mean that selectors (ids, classes etc) need to be altered too? Maybe interesting to look at additional ways to minify and cleanup the css code OOCSS for example. Or will that be a step too far for implementing in core.
Comment #3
fubhy CreditAttribution: fubhy commented... And after that we just need to move the entire Core CSS into a "Core Theme"... Please?!
Comment #4
jyve CreditAttribution: jyve commentedSubscribing. Sounds good to have a guideline to follow, especially in the world of CSS.
Comment #5
maartenverbaarschot CreditAttribution: maartenverbaarschot commentedWould be extremely nice to have a built-in patch checker for this at drupal.org. Like we currently have for PHP tests. Ideally one checks manually before uploading the patch, but it could automate part of the reviewing process.
Comment #6
mortendk CreditAttribution: mortendk commented+1 :)
Comment #7
dcmouyard CreditAttribution: dcmouyard commented+1 Sounds good to me!
Hopefully this will lead to applying OOCSS principles to Drupal.
Comment #8
tlattimore CreditAttribution: tlattimore commentedI am all for this!
+1
Comment #9
Jeff Burnz CreditAttribution: Jeff Burnz commentedCool, over the past year or so I've become a big fan of lint tools, so +1.
I don't think there is any problem getting buy-in on this issue - so maybe my initial question is scope. What I might suggest is we start running our CSS through these lint tools to get a documented list of issues - this will give us some scope and may well offer some inspiration on how we can improve on the current situation with core CSS.
What I might suggest is..
1. get a list of all core CSS
2. create a list of suitable lint tools
3. create a meta issue and break it down into separate issues per module/stylesheet
4. run the tests
5. collate the results and see what we come up with
Could even be a sprint.
Comment #10
JohnAlbinI believe that's what this issue is. :-)
I've got one of those in Zen’s drupal7-reference.css file. (Assuming no one has changed the D8 list yet.) I went ahead and put this in the initial issue description above to make tracking the changes a bit easier.
Comment #11
AdrianB CreditAttribution: AdrianB commentedSubscribing.
Comment #12
JacineThere's an initiative page for cleaning up CSS here. It'd be great to add some info about CSS Lint and the core stylesheets to that page. I'll do it if no one else gets to it first. I also think it'd be awesome to get it in our automated patch testing as @mverbaar mentioned. Tagging "Front end" to get this on the radar of people that are interested in helping out with this via @drupalfrontend.
Comment #13
steinmb CreditAttribution: steinmb commented+1
Comment #14
JacineThe page I referred to in #12 was updated after the HTML5 Initiative meeting today. @johnvsc created all the issues and linked them in a table here for those interested in helping: http://drupal.org/node/1089868
Comment #16
dodorama CreditAttribution: dodorama commentedOh yes
Comment #17
JohnAlbinI'm going to try to resurrect this issue.
Comment #18
cosmicdreams CreditAttribution: cosmicdreams commentedHappy I found this issue.
I'm going to "explode" this issue to each of the stylesheets this weekend (Saturday) if it's still a good idea. For the most part, this effort is inline with the CSS cleanup efforts of the HTML5 initiative. using CSSLint at http://csslint.net provides a quick way to code-sniff our css and tweak styles until they pass the tests.
So, as I see it, what needs to be done is:
1. Create an issue for each of the stylesheets that are mentioned in the OP
2. For each stylesheet copy and paste the it into the online tool and test it.
3. Fix any warnings or errors
4. Create a patch and see if the patch passes Drupal's automated tests.
5. Debate about the changes.
Comment #18.0
cosmicdreams CreditAttribution: cosmicdreams commentedadded link to online css lint tool.
Comment #18.1
ZenDoodles CreditAttribution: ZenDoodles commentedUpdated issue summary.
Comment #19
webchickProbably. If it's the exact same "robotic" changes just in lots of different files though, it might be easier to review/commit as a single patch. If we're talking a 500K+ patch, then probably not since the re-rolls alone would be really irritating.
Might as well try it and see what we end up with then go from there.
Comment #20
tlattimore CreditAttribution: tlattimore commentedThanks a lot for chiming in on this, webchick. I anticipate it will likely be a very large patch if all the changes a rolled into one. There has been a great deal of effort to clean things up (noted in the issue summary). But my guess is that css lint will still have a boat load of changes suggested.
Comment #20.0
tlattimore CreditAttribution: tlattimore commentedUpdated issue summary.
Comment #21
ZenDoodles CreditAttribution: ZenDoodles commentedComment #22
RobLoachI ran CSSLint through node, and this is the result.
Comment #23
droplet CreditAttribution: droplet commentedAhh. that's good.
There also a CSS Cleaup mission. Some of them are duplicated issue
http://drupal.org/node/1089868
Comment #24
barraponto CreditAttribution: barraponto commentedAren't we cleaning up the core themes CSS as well?
Comment #25
barraponto CreditAttribution: barraponto commentedbtw, here's how I roll:
find core/modules/openid/ -type f -name *.css -exec csslint {} \;
Comment #25.0
barraponto CreditAttribution: barraponto commentedUpdated issue summary.
Comment #25.1
kerasai CreditAttribution: kerasai commentedUpdated issue summary.
Comment #25.2
gregglesremoving openid issue - it's still valid in the contrib openid but not relevant for this core meta-issue.
Comment #26
LewisNymanI just created #2222049: Add a .csslintrc file that's in line with our CSS standards. It would be nice to get some opinions please
Comment #27
LewisNymanComment #28
LewisNymanSo #2222049: Add a .csslintrc file that's in line with our CSS standards is now committed which means we can use it to get better feedback from CSSlint that's inline with our standards. I've attached the patch for a grunt task you can run that pulls in the csslintrc file.
Comment #29
LewisNymanI had a look over the current status of CSS in core, here are the results:
https://gist.github.com/lewisnyman/10c756cb6f7a204485ca
Comment #30
mortendk CreditAttribution: mortendk commentedauch - well now we know what to do the next couple of weeks ;)
Comment #31
cosmicdreams CreditAttribution: cosmicdreams commentedMaking progress with ids is an interesting challenge. These ids have been deeply rooted for a long time. Should we endeavor to eliminate them?
In the meantime Lewis, it might be good to put the gulpfile you're using to automate the css linting. I assume you also have a package.json that defines how to import the necessary packages with bower.
Establishing a project like that would make the future work faster for all of us.
Comment #32
mortendk CreditAttribution: mortendk commentedID's are bad practice they always have and always will be.
Comment #33
LewisNyman@cosmicdreams Funny you should say that because I started working on something like this today ;-) https://github.com/lewisnyman/drupalcore-frontend-toolkit
I don't know if it's completely usable yet but feel free to give it a go.
Comment #34
RobLoachThere's now an HTML Report up at http://lewisnyman.co.uk/drupalcore-frontend-toolkit/ so you can get an overview.
Comment #35
tim.plunkettComment #36
mortendk CreditAttribution: mortendk commentedit should be called "Drupals css wall of shame" ;)
lewis its very usable and gives us a really good tool to find all the little things, that probably needs a cleanup (like the use of important etc)
Comment #37
mortendk CreditAttribution: mortendk commentedand for those that are running it locally heres my node csslint
csslint core --format=compact --exclude-list=core/assets/vendor/,core/modules/ckeditor/,core/vendor/ > lintcss-trap.txt
yes im ignoring venor, cause we have enough crap as it is b our self ;)
Comment #38
martin107 CreditAttribution: martin107 commentedMove, along nothing to see here... just moving the command line suggestion into the issue summary, just to save me digging around for #37 later. :-)
Comment #39
LewisNymanThe status as of February 10th:
57 lint free files out of 145
846 errors
Comment #40
LewisNymanThe status as of February 12th:
62 lint free files out of 145
802 errors
Comment #41
mortendk CreditAttribution: mortendk commented83 files on the wall - 83 files on the wall, clean one up & sent it to review
82 files on the wall ....
Comment #42
LewisNymanToday's status:
63 lint free files out of 144
789 errors
Comment #43
LewisNymanToday's status:
63 lint free files out of 144
782 errors
Comment #44
dawehnerWell, do you really want to fix every single issue with their own issue?
Comment #45
LewisNymanOur strategy at the moment is to fix individual files/components. It becomes very hard to test regressions when we have one patch that fixes one problem across all files. Some files have 40+ errors in them, so we can be quite productive still.
Comment #46
mortendk CreditAttribution: mortendk commentedwe need to go through each file & clean them up, that at the same time have the advantage that we then as well quickly gets into cleaning them up as well with the MAT filenaming & putting the right css where it belong. That should if the might Drupal gods wants it, make it easier for us to clean the css even more
Comment #47
Jeff Burnz CreditAttribution: Jeff Burnz commentedThe issue for Forum CSS linked to in the IS is postponed on an issue that itself is postponed to an issue that is now closed and didn't really appear to fix anything - the closed issue says changes were merged into the D8 mobile initiative branch but I have no idea what happened to that since those CSS changes are not in core: #2028049: Clean up the CSS for Forum module, can we re-open #1217002: Clean up the CSS for Forum module and link to in the IS?
Comment #48
LewisNymanYep that makes sense, looks like there are a few issues that were closed as duplicates that we could reopen and tag correctly?
#1217054: Clean up the CSS for User module #1217052: Clean up the CSS for Update module #1217048: Clean up the CSS for Toolbar module #1217040: Clean up the CSS for Simpletest module
Comment #49
mgiffordCan we remove #1663144: Clean up css in overlay?
Are we worried about D7 at this point in this issue?
Comment #50
LewisNyman@mgifford I guess this meta s only for 8.x so we can remove the reference for the overlay issue?
Comment #51
LewisNymanTodays status
62 lint free files out of 143
780 errors
Comment #52
LewisNymanToday's status
65 lint free files out of 144
692 errors
NIce!
Comment #53
LewisNymanToday's status
65 lint free files out of 145
690 errors
Comment #54
LewisNymanComment #55
LewisNymanComment #56
LewisNymanComment #57
LewisNymanToday's status
67 lint free files out of 144
679 errors
Comment #58
LewisNymanToday's status
71 lint free files out of 145
665 errors
Comment #59
LewisNymanComment #60
LewisNymanComment #61
LewisNymanToday's status
74 lint free files out of 145
565 errors
Down by 100 in just over a week :-) We are getting those devdays issues committed!
Comment #62
LewisNymanI updated the issue summary with a table of each module so we can see which issues need to be created
Comment #63
LewisNymanComment #64
Manjit.SinghComment #65
LewisNymanComment #66
LewisNymanComment #67
LewisNymanComment #68
Manjit.SinghComment #69
LewisNymanToday's status
79 lint free files out of 145
545 errors
We've fixed some of the smaller files but we still have a lot of files to go.
Comment #70
LewisNymanJust like that, we lose 100 errors
83 lint free files out of 145
445 errors
Comment #71
LewisNymanToday's status
86 lint free files out of 145
422 errors
We are down to about half the number of CSSLint issues we had 6 months ago. Well done everyone!
Comment #72
LewisNymanToday's status
111 lint free files out of 185
421 errors
We've added about 40 CSS files in #2395853: Split system.module.css and system.theme.css files into SMACSS style components
Comment #73
LewisNymanToday's status
114 lint free files out of 190
389 errors
New files and fixes from #2398463: Clean up the "content" component in Bartik
Comment #74
attiks CreditAttribution: attiks at Attiks commentedI've added an issue to add automated testing to the testbot #2575497: DrupalCI should report code style issues - CSS, so once all css is clean it can be enforced.
Comment #75
LewisNymanToday's status
124 lint free files out of 197
333 errors
Comment #76
elachlan CreditAttribution: elachlan commentedExtra eyes would be appreciated for #2575497: DrupalCI should report code style issues - CSS and associated child issues.
We should be in a position to switch this on as soon as core is clean.
Comment #77
LewisNymanCore won't be clean until 9.x now. We have CSSlint errors in Classy and Stable. :(
Comment #78
LewisNymanPostponing until 8.1 as we can still clean up Seven, Bartik, and module CSS.
Comment #79
cosmicdreams CreditAttribution: cosmicdreams as a volunteer commentedIs this something we can continue progress on in it's own branch?
Comment #80
LewisNyman@cosmicdreams We can make progress on the linting errors in module CSS, Bartik, and Seven for 8.1. Maybe it makes sense to work on a branch for Stable and Classy, as they are frozen until 9.x
Comment #81
LewisNymanAlso, #2575737: Copy templates, CSS, and related assets to Stable and override core libraries' CSS added around 250 csslint errors to core as it copied all the module errors over :(.
175 lint free files out of 283
573 errors
Comment #82
steinmb CreditAttribution: steinmb as a volunteer commentedDoh, that kinda suck. Getting DrupalCI to fail on those should stop this from happening again.
Comment #83
cosmicdreams CreditAttribution: cosmicdreams as a volunteer commentedOr if making it fail is too extreme perhaps list them as warnings.
Comment #84
elachlan CreditAttribution: elachlan commentedI believe the idea was to list them as warnings until core is clear of warnings. Then switch it over to failure. This might be a bit extreme for css. Nothing is set in stone yet.
Comment #85
catchIf there's no functional change, we can possibly commit these kinds of changes to patch releases, moving back for more discussion.
Comment #89
alexpottI'd be really awesome if people wanted to start to work on this again. There are hundreds of errors in HEAD - see https://www.drupal.org/pift-ci-job/634063 for example.
Comment #90
cosmicdreams CreditAttribution: cosmicdreams as a volunteer commentedWell, Midcamp is coming up! So yea, this would be an awesome issue to work on this weekend.
Comment #91
droplet CreditAttribution: droplet commentedWant to ask a quick question here before creating a new issue thread.
How about migrating it to stylelint (https://stylelint.io/). It's a better Modern tool for editors and more automatic formatting and better for now and then (e.g. SCSS in CORE, or SCSS / SASS / Whatever in Contrib)
Comment #92
cosmicdreams CreditAttribution: cosmicdreams as a volunteer commentedIt seems that stylelint.io has an npm dependency. I'm not sure if anything else we've included has a npm dependency so this looks to be the first one.
On the plus side it seems that stylelint has good PhpStorm support: https://www.jetbrains.com/help/phpstorm/2016.3/using-stylelint-code-qual... so for me that's cool.
@droplet: would we need to define a custom ruleset for the linter to follow with stylelint or will using stylelint-config-standard be sufficient.
Comment #93
cosmicdreams CreditAttribution: cosmicdreams as a volunteer commentedOh you can install stylelint globally and hook use it via the CLI or with PhpStorm/...your IDE of choice. That's cool. So nevermind about that node requirement.
Comment #94
cosmicdreams CreditAttribution: cosmicdreams as a volunteer commentedHey @droplet go ahead and create that issue for stylelint please. Also if you could, can you please show an example for what the contents of the .stylelintrc should be?
I tried this but it didn't find anything wrong with all the css.
Also, this should just scan css files right?
Comment #95
alexpott@droplet, @cosmicdreams - created the issue - #2865971: Use stylelint as opposed to csslint in core
Comment #98
joelpittetClosing this one in favour of #2865971: Use stylelint as opposed to csslint in core