Stylelint config

[mikeselander]

Adding a stylelint configuration to match the new CSS standards.

Edit by @peterwilsoncc:

TODO:

• Switch font weights to named-where-possible
• Selector regex (allowing exclusion for WP core class names, maybe in a particular directory if possible)
• Further fine tuning of line-break rules

[peterwilsoncc]

This LGTM.

@kucrut it'd be good to get your thoughts on this too, for standards we're looking to meet see the PR on the engineering repo.

[kucrut]

Looks good to me 👍

[mikeselander]

I've tested this on a client's setup and it works as expected. I'll need to update the instructions in the Engineering repo (update: done), but I think that this is ready for a final review.

[mattheu]

Looks good to me 👍

[rmccue]

Blocked on https://github.com/humanmade/Engineering/pull/85

@mikeselander So that we can publish this as a standalone rule on npm, can you put the package.json rules and config into a packages/stylelint-config-humanmade directory please? :)

[peterwilsoncc]

I'm using this on a client's site refresh to iron out a few inconsistencies and odd rules.

Still a little work to do before it can be merged.

[peterwilsoncc]

I haven't got time to dig into it right now but the following throws a bunch of errors:

background-image:
	linear-gradient(
		to bottom,
		#000 50%,
		transparent 50%
	);

wp-content/themes/bounce-2018/assets/scss/components/_site-global-nav.scss
 14:19  ✖  Expected single space after "("    function-parentheses-space-inside
 15:13  ✖  Expected single space after ","    function-comma-space-after
 16:12  ✖  Expected single space after ","    function-comma-space-after
 18:2   ✖  Expected single space before ")"   function-parentheses-space-inside

[mikeselander]

@peterwilsoncc looks like we can change those lines from always to always-single-line which should resolve the erroneous reports. I'll play with this next week or on the plane. I'm a little surprised that doesn't trigger quite a few more notices 😬

Note (for self): https://stylelint.io/user-guide/rules/function-parentheses-space-inside/

As I think about it, we also need a reliable way to test these rules out so maybe I'll give that a think as well.

[mikeselander]

@peterwilsoncc I modified the configuration file for both of the mentioned rules, could you test it out against that syntax?

Note: I moved the config into a sub-folder as requested by Ryan.

[peterwilsoncc]

Change in c12ca5c allows for comments at the top of blocks to avoid empty lines.

@mikeselander @sambulance the WP rules require numeric font-weights. As this is something CSS{nano|min} can handle, I'm tempted to remove the rule.

[sambulance]

@peterwilsoncc Wouldn't we want consistency across the source rather than the compiled CSS?

[peterwilsoncc]

@sambulance yeah, I meant reverse rather than remove so the rule would become:

"font-weight-notation" : "named-where-possible",

https://stylelint.io/user-guide/rules/font-weight-notation/

[sambulance]

👍 Do you know why WP prefers numeric values? I thought it was better to use named values so the browser can choose the correct font-weight based on what is available, rather than having to faux it (eg if a 600 weight semibold is loaded, use that as bold if a 700 weight bold font is not available).

[peterwilsoncc]

No idea why core prefers numeric weight values 🤷🏻‍♂️, there's no reason giving in their standards so I'll see if @ntwb knows the archeology.

[mikeselander]

the WP rules require numeric font-weights. As this is something CSS{nano|min} can handle, I'm tempted to remove the rule.

I personally quite like the explicitness of numbered font weights, I find it a nuisance to look up which named font size equates to which one I want but the numbered variants translate consistently and more explicitly. This is just personal preference though.

If there's some benefit that I don't know about I'm happy to be schooled :D

[mikeselander]

As another note on this, the valid values in stylelint don't look they cover enough values - Valid font-weight names are normal, bold, bolder, and lighter.. A lot of the fonts that clients use have 5-10 different weight variations.

Since I haven't used named values in quite a while (particularly with stylelint) what's the preferred way of getting around this?

[ntwb]

@mikeselander This is due to that normal and bold are the only two named weights that have numerical equivalents 400 and 700 respectively.

@peterwilsoncc @sambulance When discussing this with Peter the other day I couldn't recall the background on why switching to numerical fonts was made, and today, huzzah I've found it: https://core.trac.wordpress.org/changeset/37740. Further background can be read in the long read ticket WP#36753

[sambulance]

Thanks @ntwb that's really interesting. Seems the switch to system fonts haas found flaws with the exact reasoning I used above, but only because of the inconsistency of how the font weight is declared.

I think the bottom line is, as long as we stick to one naming convention, we should be fine. And given that named-where-possible only rejects 400 and 700, may as well go numerical.

[peterwilsoncc]

I've edited the original description to add some todos before we deploy this addressing some of the comments above and in humanmade/Engineering#85

[mikeselander]

@peterwilsoncc @rmccue how are you guys feeling about this PR? peter, do you mind expanding on the checklist that you added in the main comment?

[peterwilsoncc]

@mikeselander There are a few changes I have in Bounce that I'd like to push up next week.

Re todos:

• font-weight: crossed out because I misread the comments above
• Selector regex: stylelint allows for including regular expressions to allow/disallow certain selectors. Given how much we bike shedded this in the Engineering chat I'd like to add some rules but this can probably wait until a later version
• line breaks: There was some odd behaivour around enforcing blank lines at the start of blocks that I need to check if the current file is up to date with. I'll check on bounce

[mikeselander]

@peterwilsoncc @rmccue How would you feel about us merging this now and then adding Wilson's changes in a follow-up PR? This has been sitting for over 5 months and at some point something should be better than nothing.

[rmccue]

@mikeselander Fine by me. Long-term, do you want to own package maintenance for this? 😬

[ntwb]

I'm happy to write some tests for this once merged.

This repo also needs https://lernajs.io added.

[mikeselander]

Long-term, do you want to own package maintenance for this?

Yes, I'm happy to own maintenance on this.

I'm happy to write some tests for this once merged.

That would be amazing Stephen!

[rmccue]

This repo also needs https://lernajs.io added.

Indeed, we'll need to ensure package versions and tags are coordinated across all of these (so we'll need to immediately bump the version way up).

I'll leave this for a final approval for @peterwilsoncc, feel free to merge if you approve.

[peterwilsoncc]

👍 I'm happy if this goes in.

[mikeselander]

Nice one, people 👏