Patch #32888
closed
Use stylelint to avoid errors and enforce conventions in CSS files
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:- Adds
package.jsonandyarn.lock[1]to request the library as development dependency. To install and run it, I propose to use Yarn. - Adds
.stylelintrcwith the initial list of rules from here. For now, I propose to start only with the possible-errors rules. - Adds
.stylelintignoreto ignore some files from linting (jquery-ui-*.ccs for now). - Updates the documentation
- 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
Updated by Marius BĂLTEANU almost 5 years ago
- File 0001-Adds-Stylelint-to-lint-CSS-files.patch added
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
Updated by Marius BĂLTEANU almost 5 years ago
- File deleted (
0001-Adds-Stylelint-to-lint-CSS-files.patch)
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;
Updated by Marius BĂLTEANU almost 5 years ago
- Related to Patch #32890: Fix violations reported by Stylelint added
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.
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.
Updated by Go MAEDA almost 5 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.
Updated by Marius BĂLTEANU almost 5 years ago
Go MAEDA wrote:
Marius, let me ask one thing.
Should I commit
yarn.lockfile? I do not understand Yarn well, but it seems that it is not necessary to commit because it is generated automatically whenyarn installis 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.
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.