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
Merged
3 changes: 2 additions & 1 deletion .drone.yml
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ pipeline:
pull: true
commands:
- npm install
- make stylesheets-check
- make css
- make js
when:
event: [ push, tag, pull_request ]

Expand Down
25 changes: 25 additions & 0 deletions .eslintrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
root: true

extends:
- eslint:recommended

parserOptions:
ecmaVersion: 2015

env:
browser: true
jquery: true
es6: true

globals:
Clipboard: false
CodeMirror: false
emojify: false
SimpleMDE: false
Vue: false
Dropzone: false
u2fApi: false
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.

12 changes: 6 additions & 6 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,17 +65,17 @@ high-level discussions.
## Testing redux

Before submitting a pull request, run all the tests for the whole tree
to make sure your changes don't cause regression elsewhere.
to make sure your changes don't cause regression elsewhere.

Here's how to run the test suite:
Here's how to run the test suite:

- Install the correct version of the drone-cli package. As of this
writing, the correct drone-cli version is
[0.8.6](https://0-8-0.docs.drone.io/cli-installation/).
- Ensure you have enough free disk space. You will need at least
15-20 Gb of free disk space to hold all of the containers drone
creates (a default AWS or GCE disk size won't work -- see
[#6243](https://github.com/go-gitea/gitea/issues/6243)).
[#6243](https://github.com/go-gitea/gitea/issues/6243)).
- Change into the base directory of your copy of the gitea repository,
and run `drone exec --local --build-event pull_request`.

Expand Down Expand Up @@ -114,7 +114,7 @@ Generally, the go build tools are installed as-needed in the `Makefile`.
An exception are the tools to build the CSS and images.

- To build CSS: Install [Node.js](https://nodejs.org/en/download/package-manager) at version 8.0 or above
with `npm` and then run `npm install` and `make generate-stylesheets`.
with `npm` and then run `npm install` and `make css`.
- To build Images: ImageMagick, inkscape and zopflipng binaries must be
available in your `PATH` to run `make generate-images`.

Expand Down Expand Up @@ -214,7 +214,7 @@ to the maintainers team. If a maintainer is inactive for more than 3
months and forgets to leave the maintainers team, the owners may move
him or her from the maintainers team to the advisors team.
For security reasons, Maintainers should use 2FA for their accounts and
if possible provide gpg signed commits.
if possible provide gpg signed commits.
https://help.github.com/articles/securing-your-account-with-two-factor-authentication-2fa/
https://help.github.com/articles/signing-commits-with-gpg/

Expand Down Expand Up @@ -281,7 +281,7 @@ be reviewed by two maintainers and must pass the automatic tests.
* Create `-dev` tag as `git tag -s -F release.notes v$vmaj.$vmin.0-dev` and push the tag as `git push origin v$vmaj.$vmin.0-dev`.
* When CI has finished building tag then you have to create a new branch named `release/v$vmaj.$vmin`
* If it is bugfix version create PR for changelog on branch `release/v$vmaj.$vmin` and wait till it is reviewed and merged.
* Add a tag as `git tag -s -F release.notes v$vmaj.$vmin.$`, release.notes file could be a temporary file to only include the changelog this version which you added to `CHANGELOG.md`.
* Add a tag as `git tag -s -F release.notes v$vmaj.$vmin.$`, release.notes file could be a temporary file to only include the changelog this version which you added to `CHANGELOG.md`.
* And then push the tag as `git push origin v$vmaj.$vmin.$`. Drone CI will automatically created a release and upload all the compiled binary. (But currently it didn't add the release notes automatically. Maybe we should fix that.)
* If needed send PR for changelog on branch `master`.
* Send PR to [blog repository](https://github.com/go-gitea/blog) announcing the release.
Expand Down
49 changes: 31 additions & 18 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -365,33 +365,46 @@ release-compress:
fi
cd $(DIST)/release/; for file in `find . -type f -name "*"`; do echo "compressing $${file}" && gxz -k -9 $${file}; done;

.PHONY: javascripts
javascripts: public/js/index.js
.PHONY: js
js:
@hash npx > /dev/null 2>&1; if [ $$? -ne 0 ]; then \
echo "Please install npm version 5.2+"; \
exit 1; \
fi;
npx eslint public/js

.IGNORE: public/js/index.js
public/js/index.js: $(JAVASCRIPTS)
cat $< >| $@
.PHONY: css
css:
@hash npx > /dev/null 2>&1; if [ $$? -ne 0 ]; then \
echo "Please install npm version 5.2+"; \
exit 1; \
fi;
npx lesshint public/less/
npx -p less 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 -p less lessc --clean-css="--s0 -b" public/less/themes/$(notdir $(file)) > public/css/theme-$(notdir $(call strip-suffix,$(file))).css;)
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).


.PHONY: stylesheets-check
stylesheets-check: generate-stylesheets
@diff=$$(git diff public/css/*); \
if [ -n "$$diff" ]; then \
echo "Please run 'make generate-stylesheets' and commit the result:"; \
if ([ ! -z "$CI" ] && [ -n "$$diff" ]); then \
echo "Generated files in public/css have changed, please commit the result:"; \
echo "$${diff}"; \
exit 1; \
fi;

.PHONY: javascripts
javascripts:
echo "'make javascripts' is deprecated, please use 'make js'"
$(MAKE) js

.PHONY: stylesheets-check
stylesheets-check:
echo "'make stylesheets-check' is deprecated, please use 'make css'"
$(MAKE) css

.PHONY: generate-stylesheets
generate-stylesheets:
@hash npx > /dev/null 2>&1; if [ $$? -ne 0 ]; then \
echo "Please install npm version 5.2+"; \
exit 1; \
fi;
$(eval BROWSERS := "> 1%, last 2 firefox versions, last 2 safari versions, ie 11")
npx lesshint public/less/
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);)
echo "'make generate-stylesheets' is deprecated, please use 'make css'"
$(MAKE) css

.PHONY: swagger-ui
swagger-ui:
Expand Down
32 changes: 19 additions & 13 deletions docs/content/doc/advanced/hacking-on-gitea.en-us.md
Original file line number Diff line number Diff line change
Expand Up @@ -136,30 +136,36 @@ You should lint, vet and spell-check with:
make vet lint misspell-check
```

### Updating the stylesheets
### Updating CSS

To generate the stylsheets, you will need [Node.js](https://nodejs.org/) at version 8.0 or above.
To generate the CSS, you will need [Node.js](https://nodejs.org/) 8.0 or greater and the build dependencies:

At present we use [less](http://lesscss.org/) and [postcss](https://postcss.org) to generate our stylesheets. Do
**not** edit the files in `public/css/` directly, as they are generated from
`lessc` from the files in `public/less/`.
```bash
npm install
```

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.

**not** edit the files in `public/css` directly, as they are generated from `lessc` from the files in `public/less`.

Edit files in `public/less`, run the linter, regenerate the CSS and commit all changed files:

```bash
cd "$GOPATH/src/code.gitea.io/gitea"
npm install
make css
```

You can then edit the less stylesheets and regenerate the stylesheets using:
### Updating JS

To run the JavaScript linter you will need [Node.js](https://nodejs.org/) 8.0 or greater and the build dependencies:

```bash
make generate-stylesheets
npm install
```

You should commit both the changes to the css and the less files when making
PRs.
Edit files in `public/js` and run the linter:

```bash
make js
```

### Updating the API

Expand Down
Loading