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 autoprefixer to css build #6029

Merged
merged 4 commits into from
Feb 11, 2019
Merged

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Feb 10, 2019

- node_modules is now vendored (second commit), making it easier for contributors to run the css build as well as ensuring everyone uses the same node dependency versions.

- added make css target. Less to type and the old target is still available and will print a deprecation warning.

- added make update-node-modules which will update all node dependencies and remove unnecessary files in node_modules.

  • added autoprefixer which will automatically add CSS vendor prefixes based on a defined list of supported browsers. Removed all relevant prefixed rules from our rules.

Edit: The PR previously contained some controversial changes, removed them.

@codecov-io
Copy link

codecov-io commented Feb 10, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6029      +/-   ##
==========================================
+ Coverage   38.72%   38.72%   +<.01%     
==========================================
  Files         339      339              
  Lines       49013    49013              
==========================================
+ Hits        18980    18982       +2     
+ Misses      27287    27284       -3     
- Partials     2746     2747       +1
Impacted Files Coverage Δ
models/unit.go 14.28% <0%> (+14.28%) ⬆️

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 d0cb5aa...f897059. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 10, 2019
@silverwind
Copy link
Member Author

One rather important consideration when vendoring node_modules is that if a module or one if its dependencies includes a native compiled (and platform-specific) module (identified by a .node file anywhere in node_modules), this approach will not be portable. Currently this is not the case, but it may in the future. I'm happy to remove the vendoring if people disagree.

@adelowo
Copy link
Member

adelowo commented Feb 10, 2019

I honestly don't think it makes sense to vendor node_modules. We can leave the newer make commands but vendoring ? Not so sure.

By the way, see
https://github.com/golang/go/blob/ca7c12d4c9eb4a19ca5103ec5763537cccbcc13b/src/cmd/go/internal/modfetch/codehost/codehost.go#L31

go modules have a limit of 500MiB. I am not certain if 500MiB is for just Go source files or the entire repository size. This might be something to consider

@lafriks
Copy link
Member

lafriks commented Feb 10, 2019

I would also prefer to not have node_modules in git

@silverwind
Copy link
Member Author

I just thought it might be appreciated because the golang modules are vendored too, but I guess I'll undo it. Not a huge fan of it myself because of the mentioned native module issue.

@silverwind silverwind changed the title Vendor node_modules, add autoprefixer, add 'make css' Add autoprefixer to css build Feb 10, 2019
@silverwind
Copy link
Member Author

Reworked PR, now only includes autoprefixer. The make css change is left out as deserves its own PR.

@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 Feb 10, 2019
@lafriks lafriks added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label Feb 10, 2019
@lafriks lafriks added this to the 1.8.0 milestone Feb 10, 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 Feb 11, 2019
@techknowlogick techknowlogick merged commit 353282e into go-gitea:master Feb 11, 2019
@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.

6 participants