Initial Airbnb stylelint shared config `stylelint-config-airbnb` #23

ntwb · 2016-06-05T07:38:29Z

ntwb · 2016-06-05T07:45:50Z

A couple of notes:

I structured the folders in the same way as https://github.com/airbnb/javascript, this should facilitatte publishing the styleline-config-airbnb seperately from this repo, as is done for eslint-config-airbnb
Sorry no tests, I had to remove them as NPM was exploding and not installing packages, this can be easily revisited later, most things here in this PR are a cut and paste from https://github.com/ntwb/stylelint-config-wordpress with the tests pulled, and the config edited

DanielaValero · 2016-07-08T07:12:49Z

Hello guys,
Does any of you have time to check this pull request out?

Having an official config for the airbnb css style guide would be very helpful for the community.
We would get the benefits of it, and also would be able to make pull requests to enhance it.

@ntwb I am checking the changes you did, was looking in the Readme for instructions about how to apply the preset in our local stylelint config, but the section was missing. In case this pull request is merged, having documentation about how to use it would be quite helpful.

Thanks in advance

ljharb · 2016-07-08T07:14:47Z

packages/stylelint-config-airbnb/package.json

+    "eslint-plugin-import": "^1.8.1",
+    "eslint-plugin-jsx-a11y": "^1.2.3",
+    "eslint-plugin-react": "^5.1.1",
+    "stylelint": "^6.5.1",


should stylelint and stylelint-scss also be a peerDependency?

No, this shouldn't be needed, a few of the other stylelint configurations required the peerDependency to work around an Atom Editor issue.

Right - what I mean is, this config is likely compatible with specific versions of stylelint - making it a peer dep allows us to convey "which versions we're compatible with" in a way that fits easily with npm.

If a package wouldn't ever reasonably be installed without X, and it requires X to work, then X must be a dep or a peer dep :-)

ljharb · 2016-07-08T07:15:41Z

@ntwb Sorry for the delay on this!

Would you mind addressing these comments, and updating all the deps to the latest versions?

In addition, adding documentation as suggested by @DanielaValero would be quite helpful.

ljharb · 2016-07-08T08:27:19Z

packages/stylelint-config-airbnb/.eslintrc

@@ -0,0 +1,3 @@
+{
+  "extends": "airbnb"


let's add "root": true to this as well

ntwb · 2016-07-08T08:33:47Z

Would you mind addressing these comments, and updating all the deps to the latest versions?

@ljharb I've bumped them all apart from ESLint to 3.x per airbnb/javascript#936

ljharb · 2016-07-08T08:37:05Z

packages/stylelint-config-airbnb/package.json

+  "homepage": "https://github.com/airbnb/css#readme",
+  "devDependencies": {
+    "eslint": "^2.13.1",
+    "eslint-config-airbnb": "^9.0.1",


actually since we don't use react in this project, you could use eslint-config-airbnb-base - the latest of which does support eslint v3 :-)

ntwb · 2016-07-08T09:10:57Z

@ljharb Is that everything?

I've got more enhancements that can be added such as tests and some more SCSS rules but I'd rather look at that over the weekend and send those via PR's...

ljharb · 2016-07-08T09:12:53Z

packages/stylelint-config-airbnb/package.json

-    "eslint": "^2.13.1",
-    "eslint-config-airbnb": "^9.0.1",
+    "eslint": "^3.0.1",
+    "eslint-config-airbnb-base": "^4.0.0",
    "eslint-plugin-import": "^1.10.2",
    "eslint-plugin-jsx-a11y": "^1.5.5",
    "eslint-plugin-react": "^5.2.2",


this means the react and jsx-a11y plugins aren't needed anymore

ljharb · 2016-07-08T09:13:35Z

I don't think this will be able to be merged before next week - I need to run it by some colleagues first. It's totally fine if you want to add those things to a separate PR (or to this one if it's not merged by the time you're done).

ntwb · 2016-07-08T09:22:59Z

Cool, I'll just keep pushing commits here then 👍

Not sure if you can turn Travis CI on for this repo but now that there's a .travis.yml file I'll add some tests also.

ljharb · 2016-07-08T09:24:59Z

Good call - it's now enabled whenever there's a .travis.yml at the root. Please continue to mirror the "javascript" repo's organization :-)

ntwb · 2016-07-08T09:49:01Z

Good call - it's now enabled whenever there's a .travis.yml at the root.

At the root won't work here then will it?

Please continue to mirror the "javascript" repo's organization :-)

👍

ljharb · 2016-07-08T16:36:09Z

@ntwb The JS repo has two subpackages and a root .travis.yml file - you can probably just copy + modify the one from there.

DanielaValero · 2016-07-12T07:01:40Z

Hello @ntwb

I cloned your fork and made two commits to apply two fixes into the code, however the pull request can be made only to the original repo. Here the two relevant commits:

https://github.com/DanielaValero/css/commits/master

Also I've set up a stylelint config with all the config of airbnb, plus some best practices:

https://gist.github.com/DanielaValero/b3a72b2f3d0c85c1985d5d2d7497f347

I was trying to come up with a regex to lint the naming convention for the variables, but did not figure it out quite right.. This is the last state I could get to:

'scss/dollar-variable-pattern': '\b[a-z]+(?:-)+(\b[a-z]+(?:-))*'

ntwb · 2016-07-13T05:34:59Z

@DanielaValero Thanks, I've merged those two commits 👍

I messed up a pull request, but @DanielaValero you should be able to create pull requests against my fork and they will be added here to this pull request once merged, I think 😏

Do you want to try that with some of the changes you have in your gist?

ljharb · 2016-07-13T16:30:12Z

packages/stylelint-config-airbnb/index.js

+    'stylelint-scss',
+  ],
+  rules: {
+    'at-rule-empty-line-before': 'never',


i'm kind of surprised that this plugin doesn't have a prefix on its rule names - ie, stylelint-scss/indentation - to avoid the risk of colliding with core eslint rules :-/

It's 3am and much wine so forgive me if I interpret this a little wrong 😏

The rule at-rule-empty-line-before your comment refers to is a core stylelint rule, it is not a rule from the stylelint-scss plugin, you can see a prefixed example of this rule for that plugin here

right but it's still appearing in an eslint config, so it could still conflict with a core eslint rule. i'm saying core stylelint rules should be prefixed with "stylelint/"

I'll reiterate my previous comment, it's late, its 3am and much wine has passed 😬

This is not an ESLint configuration file, this is a stylelint configuration file

This repo has nothing to do with ESLint (aside ESLint lints the codebase)

ok that's just my confusion then :-) in that case, eslint shouldn't be a peer dependency.

DanielaValero · 2016-07-13T17:13:46Z

@ntwb sounds like a plan, I'll do a pull request on your fork

ljharb

I think we need a .travis.yml at the root that runs the scripts in the package - see https://github.com/airbnb/javascript/blob/master/.travis.yml for an example.

ljharb · 2016-12-05T22:08:04Z

packages/stylelint-config-airbnb/.travis.yml

+  directories:
+    - node_modules
+node_js:
+  - '6'


let's add 5 and 7 for completeness

Done in ea3ef3d

lencioni · 2016-12-05T22:15:26Z

Unfortunately, there are a lot of scss-lint rules that are enabled by default, so they are implicitly part of our scss-lint configuration. It would be nice if the stylelint configuration captured these. You can see the default set here: https://github.com/brigade/scss-lint/blob/master/config/default.yml

ntwb · 2016-12-05T22:30:30Z

@lencioni Not entirely sure what you mean here, there are only two SCSS lint rules in the PR currently, scss/dollar-variable-pattern and scss/at-extend-no-missing-placeholder

Any reasons as to what is what is not in this initial pull request is because there is no other documentation of the Airbnb CSS elsewhere...

I'd also like not have to continue to iterate on this one single pull request, it would be nice to get this merged initially and then build on it with further pull requests and an NPM release can then follow once the configuration is ready for launch so to speak.

lencioni · 2016-12-05T23:33:20Z

I may be misunderstanding the intent here, but if it is to produce a stylelint configuration file that would be equivalent the scss-lint configuration in this repo, then I am suggesting that it capture the spirit of all the rules that scss-lint enables by default in addition to the rules that are explicitly enabled in the scss-lint configuration in this repo. They don't need to be the same rules, but they should cover the same scenarios.

ntwb · 2016-12-05T23:51:29Z

@lencioni Cool, I'm with you now 👍

There will be a bit of time involved in going through that set of default scss-lint rules to their equivalent stylelint-css and stylelint-scss rules. I'll find a place/repo to document these so that I need only do this once and is also then available for others as a reference 💥

lencioni · 2016-12-05T23:52:40Z

If you are going through the trouble anyway, it might not be a bad idea to pull all of the default rules into the scss-lint config here as well.

ntwb · 2016-12-05T23:54:54Z

Yeah I will, it can be either in this PR, or a follow up PR, I add to add my last commit via the GutHub GUI as my local branch is full of conflicts 😖

Iteration is our friend ;)

fsmaia · 2017-04-03T14:34:49Z

Any help wanted? Would be very nice to see stylelint-config-airbnb published in NPM.

TrevorSayre · 2017-05-03T20:44:24Z

@ntwb What's the reasoning behind the blacklist for border?

    "declaration-property-value-blacklist": {
      "/^border/": ["none"]
    }

ntwb · 2017-05-03T22:33:52Z

To disallow the value none for the border properties.

There was more than likely some Airbnb code I reference that showed the preference was 0 rather than none

TrevorSayre · 2017-05-04T19:39:40Z

@ntwb I understood what the code does, was curious why you wanted to prevent none for border. However, some quick research shows that 0 is nearly always preferable save for when dealing with tables where hidden may be preferred.

Our team was preferring none because it's more "readable" although when pressed they didn't have a good functional reason. So now we do have functional reasons and have moved to 0 for most cases and hidden for tables.

I'm really looking forward to being able to point at an airbnb opinion on css linting, specifically this stylelint shared config. Thank you for your efforts!

ntwb · 2017-05-05T07:50:26Z

Thanks @TrevorSayre what I meant was I must have made that decision based off of an Airbnb code example somewhere, it also could have been a decision I made as we'd discussed this rule and the none vs 0 debate at @stylelint considerably, I honestly I cannot remember, it was so long ago 😏

TrevorSayre · 2017-05-23T20:00:39Z

@ljharb What's stalling this PR? Is there anything that still needs to be done to achieve acceptance?

ljharb · 2017-05-23T20:12:57Z

@TrevorSayre My comment here describes what's still needed.

ntwb · 2017-05-24T07:27:28Z

The Travis CI changes you requested there @ljharb were added the same day were they not?

ljharb · 2017-05-24T17:16:08Z

No, i requested "at the root" (of the repo) since travis doesn't know about "packages" - similarly to how the javascript repo does it.

ntwb · 2017-05-24T23:38:54Z

I've got a heap of merge conflicts, can you just merge this pull request so it can be iterated on please

ljharb · 2017-05-27T05:05:09Z

@ntwb then can you check the "allow edits" box on the right hand column, so i can rebase it for you?

ntwb · 2017-05-27T07:57:46Z

then can you check the "allow edits" box on the right hand column, so i can rebase it for you?

Done, I never knew that was a thing, it's an awesome thing matter of fact 👍

ljharb · 2017-06-07T03:53:31Z

k, rebased. travis.yml still needs to be fixed.

cmalard · 2017-07-28T16:46:56Z

@ntwb @ljharb Hello there, is there still something to do before merge ? :-)
Would be very nice to see stylelint-config-airbnb published in NPM 🍻

ljharb · 2017-07-29T03:32:44Z

@cmalard #23 (comment)

christophervoigt · 2017-10-04T07:52:52Z

Hey, great work so far!
Is there a chance this still gets done?

I assume the travis.yml has to be changed to something like the airbnb/javascript/.travis.yml, right?

Add NodeJS v5.x & v7.x to Travis CI configuration

ntwb · 2017-11-04T08:30:15Z

Closing in favour of #56

Initial stylelint config

c39dbce

ljharb reviewed Jul 8, 2016
View reviewed changes

docs: Add license, changelog, readme

77d9ef4

ljharb reviewed Jul 8, 2016
View reviewed changes

[Tests] Add .travis.yml

81230ec

ljharb reviewed Jul 8, 2016
View reviewed changes

refactor: Use blacklist rule to prevent border:none

445f374

ljharb reviewed Jul 13, 2016
View reviewed changes

ljharb mentioned this pull request Sep 17, 2016

Include a sass-lint configuration file #34

Closed

ntwb mentioned this pull request Dec 5, 2016

Look into a config file for using http://stylelint.io/ #13

Closed

ljharb requested changes Dec 5, 2016

View reviewed changes

ntwb mentioned this pull request Dec 6, 2016

Default SCSS shared config stylelint-scss/stylelint-scss#91

Closed

LucaPipolo mentioned this pull request Sep 7, 2017

Coding standards luca netzstrategen/bacon#8

Closed

5 tasks

christophervoigt and others added 2 commits November 1, 2017 12:52

Add NodeJS v5.x & v7.x to Travis CI configuration

26566bc

Merge pull request #2 from chlorophyllkid/patch-1

d1723d3

Add NodeJS v5.x & v7.x to Travis CI configuration

christophervoigt mentioned this pull request Nov 4, 2017

shared stylelint-config-airbnb #56

Closed

ntwb closed this Nov 4, 2017

Initial Airbnb stylelint shared config stylelint-config-airbnb #23

Initial Airbnb stylelint shared config stylelint-config-airbnb #23

Conversation

ntwb commented Jun 5, 2016

ntwb commented Jun 5, 2016

DanielaValero commented Jul 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Jul 8, 2016

Choose a reason for hiding this comment

ntwb commented Jul 8, 2016

Choose a reason for hiding this comment

ntwb commented Jul 8, 2016

Choose a reason for hiding this comment

ljharb commented Jul 8, 2016

ntwb commented Jul 8, 2016

ljharb commented Jul 8, 2016

ntwb commented Jul 8, 2016

ljharb commented Jul 8, 2016

DanielaValero commented Jul 12, 2016

ntwb commented Jul 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb Jul 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielaValero commented Jul 13, 2016

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lencioni commented Dec 5, 2016

ntwb commented Dec 5, 2016 • edited Loading

lencioni commented Dec 5, 2016

ntwb commented Dec 5, 2016

lencioni commented Dec 5, 2016

ntwb commented Dec 5, 2016

fsmaia commented Apr 3, 2017

TrevorSayre commented May 3, 2017 • edited Loading

ntwb commented May 3, 2017

TrevorSayre commented May 4, 2017 • edited Loading

ntwb commented May 5, 2017

TrevorSayre commented May 23, 2017

ljharb commented May 23, 2017

ntwb commented May 24, 2017

ljharb commented May 24, 2017

ntwb commented May 24, 2017

ljharb commented May 27, 2017

ntwb commented May 27, 2017

ljharb commented Jun 7, 2017

cmalard commented Jul 28, 2017

ljharb commented Jul 29, 2017

christophervoigt commented Oct 4, 2017 • edited Loading

ntwb commented Nov 4, 2017

Initial Airbnb stylelint shared config `stylelint-config-airbnb` #23

Initial Airbnb stylelint shared config `stylelint-config-airbnb` #23

DanielaValero commented Jul 8, 2016 •

edited

Loading

ljharb Jul 13, 2016 •

edited

Loading

ntwb commented Dec 5, 2016 •

edited

Loading

TrevorSayre commented May 3, 2017 •

edited

Loading

TrevorSayre commented May 4, 2017 •

edited

Loading

christophervoigt commented Oct 4, 2017 •

edited

Loading