916944 - Fix LESS linting

Status: RESOLVED FIXED

(Webmaker Graveyard :: Thimble, defect)

Description

[Matthew Schranz [:mjschranz]]

It's not actually linting anything.

Comment 1

[Dmitry Hamabama Yastremskiy]

In charge of checking LESS linting following module: https://github.com/sindresorhus/grunt-recess and it is used with other linting tasks. Grunt running those tasks once you run 'grunt' in Thimble directory and reads configuration from Gruntfile.js. The issue was that plugin RECESS was linting zero files, however showed it's activity. It's appear to be that Gruntfile.js had to be updated with couple lines of code to initialize RECESS and bracketing fix. 

https://github.com/mozilla/thimble.webmaker.org/pull/387

Comment 2

[Jon Buckley [:jbuck]]

Attachment: https://github.com/mozilla/thimble.webmaker.org/pull/387

Comment 3

[Pomax]

Comment on attachment 8388530 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/387

this will need to run the grunt task on the userbar-override file only, as that's the master "access point" for our LESS, rather than validating "all files" (Which would lead to things like errors on missing vars because the .less is being parsed outside of the normal import chain)

Comment 4

[Dmitry Hamabama Yastremskiy]

I've changed configuration from linting all files to linting just "userbar-overrides.less" and recess gives me the following output : 

Running "recess:dist" (recess) task
>> '@{language-bower-path}/webmaker-language-picker/styles/languages.less' wasn't found.
Warning:  Used --force, continuing.
Warning: undefined: '@{language-bower-path}/webmaker-language-picker/styles/languages.less' wasn't found.
 of undefined

After some research I realized it is known issue that variables not understood in import paths. For example, this issue https://github.com/twitter/recess/issues/143 explains similar issue and it has no response for 20 days. Also contribution's graphic shows that nobody has contributed to this repo since October 2013. Perhaps this issue won't be resolved quickly, so I looked for another solution. 
I found this one: http://kevinsawicki.github.io/grunt-lesslint/   which does LESS linting as well. Tests showing that this app understands variables in paths. This is output of the app:

Running "lesslint:src" (lesslint) task
public/stylesheets/userbar-overrides.less (14)
Element (a#current-language) is overqualified, just use #current-language without element name. Don't use classes or IDs with elements (a.foo or a#foo). (overqualified-elements)
>> public/stylesheets/userbar-overrides.less 18: a#current-language {
Don't use IDs in selectors. Selectors should not contain IDs. (ids)
>> public/stylesheets/userbar-overrides.less 18: a#current-language {
>> public/stylesheets/userbar-overrides.less 27: #webmaker-nav {
>> public/stylesheets/userbar-overrides.less 55: #webmaker-nav .loginbutton {
>> public/stylesheets/userbar-overrides.less 69: #webmaker-nav .loginbutton .icon {
>> public/stylesheets/userbar-overrides.less 82: #webmaker-nav .loginbutton .label {
>> public/stylesheets/userbar-overrides.less 89: #webmaker-nav .loginbutton .tip {
>> public/stylesheets/userbar-overrides.less 101: #webmaker-nav .loginbutton,
>> public/stylesheets/userbar-overrides.less 101: #webmaker-nav .loginbutton,
>> public/stylesheets/userbar-overrides.less 106: #webmaker-nav .logoutbutton .label:hover {
Using height with border-bottom can sometimes make elements larger than you expect. Don't use width or height when using padding or border. (box-model)
>> public/stylesheets/userbar-overrides.less 28: border-bottom: 1px solid lightgrey;
Element (a.logo) is overqualified, just use .logo without element name. Don't use classes or IDs with elements (a.foo or a#foo). (overqualified-elements)
>> public/stylesheets/userbar-overrides.less 32: .webmaker-nav-container a.logo {
Don't use adjoining classes. (adjoining-classes)
>> public/stylesheets/userbar-overrides.less 38: .webmaker-nav-container .webmaker-logo-icon.webmaker-icon {
Use of !important Be careful when using !important declaration (important)
>> public/stylesheets/userbar-overrides.less 52: display: none!important;

>> 14 lint errors in 1 file.
Warning: Task "lesslint:src" failed. Used --force, continuing.


I'm not sure if these errors are relevant. Please suggest me what to do next. Thank you.

Comment 5

[Pomax]

we are not going to use the default LESS rules, we're going to tell it we think it's wrong, and that we want to definitely use things like ids, overqualification, etc. 

Have a look at https://github.com/mozilla/thimble.webmaker.org/blob/master/Gruntfile.js for an example of how we do this in thimble.

Comment 6

[Pomax]

Comment on attachment 8388530 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/387

in order for this to land, the first thing that needs to happen is to set up rules so that the tests pass, otherwise our tools don't pass Travis CI testing, which means we can't (reasonably) push to staging and production.

Rather than "fix the CSS/LESS", first add in all these handy techs and set up their rules so that there are no errors. Then we can file follow-up tickets to take out the "too lax" rules and adjust the css/less accordingly so that with that laxness taken out, the css/less still passes.

Comment 7

[Dmitry Hamabama Yastremskiy]

I've added rules for LESSLINT, but encountered problem with paths. In userbar-overrides.less used variable @language-bower-path to import webmaker-language-picker/style/languages.less and in languages.less used the same variable to import hint.css, but the actual path changes depends on location, so I had to create additional variable that holds path for hint.css. I also added variable @colors-path in webmaker-language-picker/style/languages.less for 'colors' importing, because 'colors' location changes depends on project.

https://github.com/mozilla/webmaker-language-picker/pull/2

@colors-path defined in userbar-overrides.less.

Comment 8

[Pomax]

Comment on attachment 8388530 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/387

this seems to work, but please verify that running thimble with these .less paths actually works, too =)

Comment 9

[Dmitry Hamabama Yastremskiy]

Tested on webmaker-suite. Didn't notice any issues. Is the PR now eligible to be landed? :)

Comment 10

[Dmitry Hamabama Yastremskiy]

Updated to the latest version of webmaker-langiage-picker. Please consider to land. Thank you.

Comment 11

[Dmitry Hamabama Yastremskiy]

Rebased! Please consider to land.