-
- Notifications
You must be signed in to change notification settings - Fork 6.2k
Compress css with nodejs #2580
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
Compress css with nodejs #2580
Conversation
Codecov Report
@@ Coverage Diff @@ ## master #2580 +/- ## ======================================= Coverage 27.33% 27.33% ======================================= Files 86 86 Lines 17137 17137 ======================================= Hits 4684 4684 Misses 11775 11775 Partials 678 678 Continue to review full report at Codecov.
|
9d11b26
to e3bb6d1
Compare public/css/index.css Outdated
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.
why recovery this css file?
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.
Put the unminify content on git should be friendly to develpers, so I agree with this change.
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.
index.css file should be left minimized
Makefile Outdated
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.
Maybe rename this build step to minify-stylesheets
?
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.
@daviian this step will later be used also to minify js files etc, so can be left as is
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.
@lafriks That's exact reason for that name.
I'm somehow against it. Development should be done in |
@lafriks @lunny There are few reason why I think css should be unminified til build process:
Another question: If we add JS to "minify", would you like to have 2 different workflows for JS and CSS? |
|
As for your question it should be same workflow. |
@lafriks OK, I agree with you about no need of not minified version of css. Now look at #2581. Stylesheets test is based on differing before and after generating it from less file. But For JS, you want to create *.min.js from *.js and use it inside template? What about development, debug? |
LGTM |
Minify can be changed to nodejs version, I see no problem with that. As for js files there should be single js file built from multiple js files. Both minified and unminified can be built and as we already have mode in app.ini (production/development) that can be used to include either minified or unminified version |
I forgot about modes app.ini. I'll try do some changes. |
e3bb6d1
to 48cd7da
Compare @lafriks Changed PR to just minify css with nodejs instead in single step. |
LGTM |
Minify css only when building gitea so we can develop, debug and compare changes. JS should be add in the future.
Also needed for changes in building gitea with drone - including docker image with node instead of installing node on drone build.