Add stylelint #43

xPaw · 2016-02-14T22:26:55Z

Fixes #10

astorije · 2016-02-14T22:33:28Z

client/themes/crypto.css

@@ -9,75 +9,79 @@ GitHub: https://github.com/aynik
 */

 @font-face {
-    font-family: Inconsolata-g;
-    src: url("fonts/inconsolatag.woff") format("woff"), url("fonts/inconsolatag.ttf") format("ttf");
+	font-family: Inconsolata-g;


Uuuurgh, no, please no... :-)

client/css/style.css is 2-space indentation, let's use that instead of tabs please. Plus it will reduce the change list a fair bit!

Good point raised by @xPaw on IRC is that style.css is not even 2-space...

astorije · 2016-02-14T23:25:14Z

@JocelynDelalande, stylelint breaks Node 0.10. Considering how far behind Node 0.10 is (even I am behind and I'm using 0.12 for https://github.com/astorije/ansible-role-shout/ ...), is it reasonable to remove support with 0.10 for The Lounge?
Reason I'm asking is because Debian Jessie still ships with 0.10...

dgw · 2016-02-14T23:28:20Z

Thing is, if you wait for Debian stable to be updated, you'll be supporting ancient versions of things for literally years.

astorije · 2016-02-14T23:29:44Z

I know, I'm just looking for the sensible thing to do, and when it comes to Debian, @JocelynDelalande is clearly the right person to ask :-)

xPaw · 2016-02-15T08:57:41Z

Keep in mind 0.10 won't work for running stylelint, lounge itself should continue working.

AlMcKinlay · 2016-02-15T09:07:17Z

No matter how far behind a distro is, everything can run nvm, so there is no excuse for running an old version of node. I would be ok with adding it even if it breaks 0.10. 0.10 came out 3 years ago, and 0.12 came out last year. And bearing in mind we have 4 (lts) and 5 (stable) since then, I don't think it's unreasonable to limit it to 0.12 and above.

I understand wanting to keep everything working, but we really restrict ourselves in that case, for example if we ever want to use ES6 features, we'll either need to use babel (which is quite heavy) or drop support for 0.12.

AlMcKinlay · 2016-02-15T09:13:29Z

Just for reference, apparently node are supporting 0.10 in maintenance till October and 0.12 till December. We don't think twice about not supporting old versions of browsers that are still in maintenance, so why would node be any different?

xPaw · 2016-02-15T10:36:48Z

Yeah, I agree with dropping 0.x versions from travis builds. If lounge will continue to work on these versions, then good for them.

JocelynDelalande · 2016-02-15T12:31:42Z

Yeah, I agree with dropping 0.x versions from travis builds. If lounge will continue to work on these versions, then good for them.

In something CI-driven, that's not a matter of dropping them from travis, that means dropping support.

Thinking about this message :

Keep in mind 0.10 won't work for running stylelint, lounge itself should continue working.

I agree, as it means we don't need to drop support of 0.10 for this very PR. Skipping stylelint for 0.10 (or even 0.x) test suite seems better to me. No user lost, no complicated setup, no loss in quality neither.

What do you think ?

I'd be happy to discuss node versions support, debian ubuntu and folks in a dedicated topic then :-) (and yes, I may be the conservative guy, for good reasons ;-).

AlMcKinlay · 2016-02-15T15:26:25Z

I'm not suggesting dropping 0.10 overall. (nor am I suggesting keeping this enables if it breaks 0.10). What I'm suggesting ist hat if we have something that can either be for 4/5 and not 0.10/0.12 or overly complex, I would have no objections to dropping 0.10/0.12 support.

JocelynDelalande · 2016-02-15T18:16:40Z

@YaManicKill

So what do you think about that proposal (regarding this PR) :

I agree, as it means we don't need to drop support of 0.10 for this very PR. Skipping stylelint for 0.10
(or even 0.x) test suite seems better to me. No user lost, no complicated setup, no loss in quality neither.

AlMcKinlay · 2016-02-15T19:05:09Z

Yeah, I'm happy with that for this pr. It'll still fail on all other
builds, and its not like it can cause issues not checking on 0.10 only.

On Mon, 15 Feb 2016, 18:16 JocelynDelalande notifications@github.com
wrote:

@YaManicKill https://github.com/YaManicKill

So what do you think about that proposal (regarding this PR) :

I agree, as it means we don't need to drop support of 0.10 for this very
PR. Skipping stylelint for 0.10
(or even 0.x) test suite seems better to me. No user lost, no complicated
setup, no loss in quality neither.

—

Reply to this email directly or view it on GitHub
#43 (comment).

astorije · 2016-02-16T00:18:20Z

We don't think twice about not supporting old versions of browsers that are still in maintenance, so why would node be any different?

It's a bit different, actually. On a laptop, you are supposed to update your software as frequently as possible. As a matter of fact, recent browsers auto-update. On a server, every upgrade decision is a long, very long process that involves countless tests, upgrade plans, ... Plus node comes installed in default packages on Debian and Ubuntu, which is what lots of admins rely on (for security and stability reasons, among others...).
Yes, it's a pain :-(

And bearing in mind we have 4 (lts) and 5 (stable) since then, I don't think it's unreasonable to limit it to 0.12 and above.
I understand wanting to keep everything working, but we really restrict ourselves in that case, for example if we ever want to use ES6 features, we'll either need to use babel (which is quite heavy) or drop support for 0.12.

My following statement being said, I fully agree that support should be kept only for the LTS and Stable versions (resp. 4 and 5)! Partly because fully in favor of ES6 (because it's 2016!) but also because that's the whole point of a LTS! Also, wasn't there an unresolved vulnerability with Node.js? (Hmmm, actually I think it was Express...)
We need however to be mindful of serious Debian admins though (I install Node.js loosely outside of apt-get so I don't consider myself serious :-)

I agree, as it means we don't need to drop support of 0.10 for this very PR. Skipping stylelint for 0.10 (or even 0.x) test suite seems better to me. No user lost, no complicated setup, no loss in quality neither.

Actually, we can't (and we shouldn't be able to!) do that. The way our Travis CI config works right now is that it runs npm test which in turn runs tests && js lint && css lint. It's a good thing to hide everything you want to check behind npm test because node developers are used to run npm test to run their tests.
We could make the CI configuration overly complex to take care of this but, really, it is much better to remove 0.10 from the Travis CI configuration. When we have code coverage over 0.01% then, sure, we can think of testing platform integration as much as we can, but as long as Travis CI is mostly (if not only) used to run our linters, then removing 0.10 from the config is the right way to go.
Think of the alternative of making the CI conf more complicated: it will essentially tell Node.js to run that one tests and the 2 linters minus 1 of the 2 linters.
Clearly, we are overthinking this. My final stance is: if we want this, we drop 0.10 from the CI.

JocelynDelalande · 2016-02-16T14:55:25Z

Actually, we can't (and we shouldn't be able to!) do that. The way our Travis CI config works right now is that it runs npm test which in turn runs tests && js lint && css lint. It's a good thing to hide everything you want to check behind npm test because node developers are used to run npm test to run their tests.

Ok, actually, I supposed tests suites were run individualy by travis, which would have made my suggestion simpler.

We could make the CI configuration overly complex to take care of this but, really, it is much better to remove 0.10 from the Travis CI configuration. When we have code coverage over 0.01% then, sure, we can think of testing platform integration as much as we can, but as long as Travis CI is mostly (if not only) used to run our linters, then removing 0.10 from the config is the right way to go.

For now, I agree with you.

Think of the alternative of making the CI conf more complicated: it will essentially tell Node.js to run that one tests and the 2 linters minus 1 of the 2 linters.
Clearly, we are overthinking this. My final stance is: if we want this, we drop 0.10 from the CI.

To go forward, I'm ok for now, but as soon as we have better coverage, the issue will arise again.

I do not add anything to the node version debate, on purpose, waiting for dedicated discussion.

This is needed for #43 to pass the CI.

astorije · 2016-02-18T05:13:38Z

(Alright, taking ownership and first PR of this too...)

Argh, somehow I can't comment on the style.css file directy... so doing it here.

I saw a few of these:

-opacity: .8;
+opacity: 0.8;

Is it something we have more in style.css than themes (btw I believe style.css should show the example whenever possible)?
FYI this is fully authorized by the CSS Values and Units Module Level 3 spec:

A number is either an or zero or more decimal digits followed by a dot (.) followed by one or more decimal digits [...]

I'm in favor of this .8 form, but not heavily opposed if you have a strong case :-)

Apart from this, I'm all good with this and the choices, very cool PR!

astorije · 2016-02-18T05:21:26Z

package.json

@@ -14,7 +14,7 @@
    "start": "node index",
    "build": "grunt",
    "test": "HOME=test/fixtures mocha test/**/*.js && npm run lint",
-    "lint": "eslint ."
+    "lint": "eslint . && stylelint \"**/*.css\""


I don't think you need to wrap the selector with double-quotes, do you?

xPaw · 2016-02-18T09:25:44Z

Is it something we have more in style.css than themes

I think it might have been .0s everywhere. However 0.0s feels more explicit, if you know what I mean. Anyone else got opinions on this?

AlMcKinlay · 2016-02-18T09:43:08Z

Personally, I prefer the more explicit 0.x rather than just .x

astorije · 2016-02-20T17:06:46Z

@xPaw: I think it might have been .0s everywhere. However 0.0s feels more explicit, if you know what I mean. Anyone else got opinions on this?
@YaManicKill: Personally, I prefer the more explicit 0.x rather than just .x

~~Meh, 2 against 1, I guess I'll surrender then :-/~~
However, this is not less explicit than not specifying 0 instead of 0px, #ccc instead of #cccccc, etc.
As a matter of fact, it's very common to specify it that way, like the unminified Bootstrap CSS does.
So... Are you sure? :-)

@xPaw, could you rebase please? :-/

astorije · 2016-02-20T17:17:01Z

I amend my last comment: between well-distributed libs like Bootstrap, and CSS experts using this notation (ex: http://lea.verou.me/wp-content/themes/elegant/style.css), I do believe .5 this is the most common thing to do in the current CSS world.

xPaw · 2016-02-20T17:45:40Z

It's back to .5s style.

astorije · 2016-02-20T18:02:21Z

👍

AlMcKinlay · 2016-02-21T15:33:52Z

Yeah, sure, I can live with that. I still prefer 0.x, as I think it's easy to miss the "." when it's at beginning, but not enough to argue about it.

👍 merging.

Add stylelint

astorije reviewed Feb 14, 2016
View reviewed changes

xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Feb 14, 2016

astorije added a commit that referenced this pull request Feb 17, 2016

Remove Node 0.10 from Travis CI

42a607d

This is needed for #43 to pass the CI.

This was referenced Feb 17, 2016

Remove Node 0.10 from Travis CI #60

Merged

Document required nodejs version erming/shout#651

Closed

astorije reviewed Feb 18, 2016
View reviewed changes

astorije self-assigned this Feb 18, 2016

xPaw added 2 commits February 20, 2016 19:32

Add stylelint

4be80ed

Update css files to confront stylelint rules

ad6b4e8

astorije added the second review needed label Feb 20, 2016

AlMcKinlay added a commit that referenced this pull request Feb 21, 2016

Merge pull request #43 from xPaw/stylelint

ac4db3a

Add stylelint

AlMcKinlay merged commit ac4db3a into thelounge:master Feb 21, 2016

AlMcKinlay removed the second review needed label Feb 21, 2016

xPaw deleted the stylelint branch March 7, 2016 17:17

astorije added this to the 1.2.0 milestone Apr 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stylelint #43

Add stylelint #43

xPaw commented Feb 14, 2016

astorije Feb 14, 2016

astorije Feb 14, 2016

astorije commented Feb 14, 2016

dgw commented Feb 14, 2016

astorije commented Feb 14, 2016

xPaw commented Feb 15, 2016

AlMcKinlay commented Feb 15, 2016

AlMcKinlay commented Feb 15, 2016

xPaw commented Feb 15, 2016

JocelynDelalande commented Feb 15, 2016

AlMcKinlay commented Feb 15, 2016

JocelynDelalande commented Feb 15, 2016

AlMcKinlay commented Feb 15, 2016

astorije commented Feb 16, 2016

JocelynDelalande commented Feb 16, 2016

astorije commented Feb 18, 2016

astorije Feb 18, 2016

xPaw commented Feb 18, 2016

AlMcKinlay commented Feb 18, 2016

astorije commented Feb 20, 2016

astorije commented Feb 20, 2016

xPaw commented Feb 20, 2016

astorije commented Feb 20, 2016

AlMcKinlay commented Feb 21, 2016

Add stylelint #43

Add stylelint #43

Conversation

xPaw commented Feb 14, 2016

astorije Feb 14, 2016

Choose a reason for hiding this comment

astorije Feb 14, 2016

Choose a reason for hiding this comment

astorije commented Feb 14, 2016

dgw commented Feb 14, 2016

astorije commented Feb 14, 2016

xPaw commented Feb 15, 2016

AlMcKinlay commented Feb 15, 2016

AlMcKinlay commented Feb 15, 2016

xPaw commented Feb 15, 2016

JocelynDelalande commented Feb 15, 2016

AlMcKinlay commented Feb 15, 2016

JocelynDelalande commented Feb 15, 2016

AlMcKinlay commented Feb 15, 2016

astorije commented Feb 16, 2016

JocelynDelalande commented Feb 16, 2016

astorije commented Feb 18, 2016

astorije Feb 18, 2016

Choose a reason for hiding this comment

xPaw commented Feb 18, 2016

AlMcKinlay commented Feb 18, 2016

astorije commented Feb 20, 2016

astorije commented Feb 20, 2016

xPaw commented Feb 20, 2016

astorije commented Feb 20, 2016

AlMcKinlay commented Feb 21, 2016