-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add make targets for js and css, add js linter #6952
Conversation
- add `make js`, deprecating `make javascripts` - add `make css`, deprecating `make generate-stylesheets` and `make stylesheets-check` - changed the unclean css check to only run on CI - add JS linting via eslint with basic configuration and fixed discovered issues - changed autoprefixer to use official `postcss-cli` avoiding the need to loop in the makefile - moved browserslist to package.json so other future tools can use it too. - update documentation for new make targets and added JS section
hljs: false | ||
|
||
rules: | ||
no-unused-vars: [error, {args: all, argsIgnorePattern: ^_, varsIgnorePattern: ^_, ignoreRestSiblings: true}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to remove this rule customization, but there are a few cases of unused function args and marking them with a _
prefix is somewhat commonplace to indicate they are unused. I think the only other option is to disable that rule.
npx lessc --clean-css="--s0 -b" public/less/index.less public/css/index.css | ||
$(foreach file, $(filter-out public/less/themes/_base.less, $(wildcard public/less/themes/*)),npx lessc --clean-css="--s0 -b" public/less/themes/$(notdir $(file)) > public/css/theme-$(notdir $(call strip-suffix,$(file))).css;) | ||
$(foreach file, $(wildcard public/css/*),npx postcss --use autoprefixer --autoprefixer.browsers $(BROWSERS) -o $(file) $(file);) | ||
npx postcss --use autoprefixer --no-map --replace public/css/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had some weird issue with the foreach
on one machine so I switched to the official postcss
cli which can process multiple files (and which is also more powerful).
Codecov Report
@@ Coverage Diff @@
## master #6952 +/- ##
==========================================
- Coverage 41.47% 41.47% -0.01%
==========================================
Files 441 441
Lines 59662 59662
==========================================
- Hits 24745 24744 -1
- Misses 31694 31695 +1
Partials 3223 3223
Continue to review full report at Codecov.
|
Two small fixups pushed. The second one eliminates the |
|
||
If you wish to work on the stylesheets, you will need to install `lessc` the | ||
less compiler and `postcss`. The recommended way to do this is using `npm install`: | ||
At present we use [less](http://lesscss.org/) and [postcss](https://postcss.org) to generate our CSS. Do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't mind if we added mention of what needs to be installed directly on this page in addition to just linking to the pages of those projects.
npm install less -g
npm install postcss
npm install clean-css
(Or whatever the appropriate commands are if not those)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
npx
is used, so the user won't need to install anything globally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
npm install
takes care of it all. It installs bin scripts into node_modules/.bin
which npx
then calls directly.
If the user did not run npm install
it will on-demand fetch the binary from the registry, so the commands should work in all cases as long as the user has a working internet connection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh ok my bad I thought I had run npm install and then didn't. FWIW since I had not, it did fail with this:
[mrsdizzie@mbp ~/.go/src/code.gitea.io/gitea (pr-6952-1557933284) ] > make css
npx lesshint public/less/
npx: installed 178 in 5.333s
npx lessc --clean-css="--s0 -b" public/less/index.less public/css/index.css
npm ERR! code E404
npm ERR! 404 Not Found - GET https://registry.npmjs.org/lessc - Not found
npm ERR! 404
npm ERR! 404 'lessc@latest' is not in the npm registry.
npm ERR! 404 You should bug the author to publish it (or use the name yourself!)
npm ERR! 404
npm ERR! 404 Note that you can also install from a
npm ERR! 404 tarball, folder, http url, or git url
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this is a bug. The package less
provides binary lessc
, so we should actually use npx less
. Investigating possible fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems npx -p less lessc --clean-css...
didn't work without npm i
due to plugin for lessc not being installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which plugin is missing? Maybe try multiple -p
arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically less-plugin-clean-css
. I'm going to pull down the branch and attempt multiple -p
args.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a fix with multiple -p
, was also necessary for postcss.
Now because of the mentioned bug with -p
, modules are installed every execution even if node_modules
is present. I'm thinking maybe I should just re-add the check for presence of node_modules
and if its there, assume bin scripts work via regular npx
calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed the fix with the check for node_modules
. I think this is the best compromise as the user only has to install modules once and subsequent runs of make css
and make js
will be fast that way and we can be sure that all plugins and stuff will be there as well.
This reverts commit 119b725.
* add make targets for js,css, add javascript linter - add `make js`, deprecating `make javascripts` - add `make css`, deprecating `make generate-stylesheets` and `make stylesheets-check` - changed the unclean css check to only run on CI - add JS linting via eslint with basic configuration and fixed discovered issues - changed autoprefixer to use official `postcss-cli` avoiding the need to loop in the makefile - moved browserslist to package.json so other future tools can use it too. - update documentation for new make targets and added JS section * fix indentation * move functions used in html to 'exported' list * Run lessc binary without having to install anything to node_modules * use relative paths to node bin scripts, removing npx * Revert "use relative paths to node bin scripts, removing npx" This reverts commit 119b725. * fix lessc and postcss plugins * check for node_modules and use actual bin names
make js
, deprecatingmake javascripts
make css
, deprecatingmake generate-stylesheets
andmake stylesheets-check
postcss-cli
avoiding the need to loop in the makefile