Skip to content
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

Merged
merged 11 commits into from
May 16, 2019

Conversation

silverwind
Copy link
Member

  • 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

- 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}]
Copy link
Member Author

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.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 14, 2019
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/*
Copy link
Member Author

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-io
Copy link

codecov-io commented May 14, 2019

Codecov Report

Merging #6952 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
modules/log/colors_router.go 83.33% <0%> (-4.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 775a5a5...9cee9f8. Read the comment docs.

@techknowlogick techknowlogick added the topic/ui Change the appearance of the Gitea UI label May 15, 2019
@techknowlogick techknowlogick added this to the 1.9.0 milestone May 15, 2019
@lafriks lafriks added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label May 15, 2019
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 15, 2019
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 15, 2019
@silverwind
Copy link
Member Author

silverwind commented May 15, 2019

Two small fixups pushed. The second one eliminates the eslint-disable comments by moving the functions used in HTML to exported.


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
Copy link
Member

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)

Copy link
Member

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.

Copy link
Member Author

@silverwind silverwind May 15, 2019

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.

Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

@lafriks lafriks merged commit d9dcd09 into go-gitea:master May 16, 2019
@lafriks lafriks removed the topic/ui Change the appearance of the Gitea UI label May 16, 2019
jeffliu27 pushed a commit to jeffliu27/gitea that referenced this pull request Jul 18, 2019
* 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
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants