Project

General

Profile

Actions

Patch #32888

closed

Use stylelint to avoid errors and enforce conventions in CSS files

Added by Marius BĂLTEANU almost 5 years ago. Updated over 4 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Code cleanup/refactoring
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:

Description

In the last few months we had some cases when we added/fixed broken CSS code (ex: r18173, r18144, #32838). These types of errors can be easily avoid if we use a lint tool for CSS (as we started using Rubocop for Ruby).

My proposal is to add Stylelint which is a very well know library, actively maintained, with good community and integrated by many tools.

Attached is a patch that:
  1. Adds package.json and yarn.lock[1] to request the library as development dependency. To install and run it, I propose to use Yarn.
  2. Adds .stylelintrc with the initial list of rules from here. For now, I propose to start only with the possible-errors rules.
  3. Adds .stylelintignore to ignore some files from linting (jquery-ui-*.ccs for now).
  4. Updates the documentation
Beside this, if the overall idea is accepted, I have in plan to add the following:
  • Add more rules to enforce some coding standards in CSS files
  • Add a Javascript lint tool like ESLint
  • Use package.json to handle the version of the current JS libraries (ex: jQuery, jQuery UI, Chart, Raphael, Tribute, etc)
  • Give a shoot to integrate Webpack as assets pipeline (In Rails 6, is default)

Any feedback is really appreciated.

1 I prefer to have yarn.lock committed in the repository in order to enforce the same version between Redmine installations.


Files


Related issues

Related to Redmine - Patch #32890: Fix violations reported by StylelintClosedGo MAEDA

Actions
Actions #1

Updated by Marius BĂLTEANU almost 5 years ago

  • File 0001-Adds-Stylelint-to-lint-CSS-files.patch added
Actions #2

Updated by Marius BĂLTEANU almost 5 years ago

Running Stylelint over the existing CSS code from public folder, returns the following violations:

root@df2fd1389bf4:/work# node_modules/.bin/stylelint "public/stylesheets/**/*.css" 

public/stylesheets/application.css
  561:1   ✖  Unexpected duplicate selector "div#issue-changesets div.changeset", first used at line 560   no-duplicate-selectors                   
  682:77  ✖  Unexpected duplicate "white-space"                                                           declaration-block-no-duplicate-properties
 1084:1   ✖  Unexpected duplicate selector "table.progress", first used at line 1075                      no-duplicate-selectors                   
 1560:1   ✖  Unexpected duplicate selector ".sort-handle", first used at line 706                         no-duplicate-selectors                   
 1619:83  ✖  Unexpected duplicate "color"                                                                 declaration-block-no-duplicate-properties

public/stylesheets/responsive.css
 137:5   ✖  Unexpected duplicate "height"                                                                                                           declaration-block-no-duplicate-properties    
 264:18  ✖  Unexpected missing generic font family                                                                                                  font-family-no-missing-generic-family-keyword
 399:5   ✖  Unexpected duplicate "display"                                                                                                          declaration-block-no-duplicate-properties    
 400:5   ✖  Unexpected duplicate "display"                                                                                                          declaration-block-no-duplicate-properties    
 401:5   ✖  Unexpected duplicate "display"                                                                                                          declaration-block-no-duplicate-properties    
 423:5   ✖  Unexpected duplicate "display"                                                                                                          declaration-block-no-duplicate-properties    
 424:5   ✖  Unexpected duplicate "display"                                                                                                          declaration-block-no-duplicate-properties    
 425:5   ✖  Unexpected duplicate "display"                                                                                                          declaration-block-no-duplicate-properties    
 492:5   ✖  Unexpected duplicate "display"                                                                                                          declaration-block-no-duplicate-properties    
 493:5   ✖  Unexpected duplicate "display"                                                                                                          declaration-block-no-duplicate-properties    
 494:5   ✖  Unexpected duplicate "display"                                                                                                          declaration-block-no-duplicate-properties    
 510:5   ✖  Unexpected duplicate "height"                                                                                                           declaration-block-no-duplicate-properties    
 804:3   ✖  Unexpected duplicate selector "#issue_tree .issue > td:not(.checkbox), #relations .issue > td:not(.checkbox)", first used at line 787   no-duplicate-selectors                       
 828:3   ✖  Unexpected duplicate selector "#issue_tree .issue > td.subject,   #relations .issue > td.subject", first used at line 799               no-duplicate-selectors                       

public/stylesheets/rtl.css
 322:3   ✖  Unexpected duplicate "margin-left"    declaration-block-no-duplicate-properties
 322:21  ✖  Unexpected duplicate "margin-right"   declaration-block-no-duplicate-properties
 377:14  ✖  Unexpected empty block                block-no-empty
Actions #3

Updated by Marius BĂLTEANU almost 5 years ago

  • File deleted (0001-Adds-Stylelint-to-lint-CSS-files.patch)
Actions #4

Updated by Marius BĂLTEANU almost 5 years ago

Updated the patch to ignore consecutive duplicated properties with different values in order to not trigger a violation for the following case:

display: -webkit-flex;
display: -ms-flexbox;
display: -webkit-box;
display: flex;
Actions #5

Updated by Marius BĂLTEANU almost 5 years ago

  • Related to Patch #32890: Fix violations reported by Stylelint added
Actions #6

Updated by Go MAEDA almost 5 years ago

  • Target version changed from Candidate for next major release to 4.2.0

I think it is nice to have Stylelint. Setting the target version to 4.2.0.

Actions #7

Updated by Bernhard Rohloff almost 5 years ago

It's a good idea. And I'm completely with you Marius that we need some coding standards on the stylesheets.

Actions #8

Updated by Go MAEDA over 4 years ago

Marius, let me ask one thing.

Should I commit yarn.lock file? I do not understand Yarn well, but it seems that it is not necessary to commit because it is generated automatically when yarn install is executed. If I commit this file, I have to keep updating it in the future.

Actions #9

Updated by Marius BĂLTEANU over 4 years ago

Go MAEDA wrote:

Marius, let me ask one thing.

Should I commit yarn.lock file? I do not understand Yarn well, but it seems that it is not necessary to commit because it is generated automatically when yarn install is executed. If I commit this file, I have to keep updating it in the future.

Yes, it should be committed in the source control and updated when we will do changes to packages.json. It's a good practice to commit the lock file under source control, regardless the language/framework (Ruby, PHP, Javascript) in order to ensure the same version across all servers/installations/machines and avoid issues generated by new packages version (Redmine has a lot of issues generated by this case).

The install command first checks for the lock file and if exists, it will install the same version of the packages. If doesn't exist, it will be generated based on which versions are defined in the *.json and available at that moment.

https://classic.yarnpkg.com/en/docs/yarn-lock/#toc-check-into-source-control

In Redmine, from what I known, the Gemfile.lock is not committed because we support multiple environments and platforms, but for Javascript packages is not the case.

Actions #10

Updated by Go MAEDA over 4 years ago

  • Status changed from New to Closed
  • Assignee set to Go MAEDA

Committed the patch. Thank you for improving the dev environment.

Actions

Also available in: Atom PDF