feat: add convert less css file step. #1861

appleboy · 2017-06-03T07:34:54Z

Add convert less css file in drone CI/CD step.

silverwind · 2017-06-03T07:50:48Z

I'd go the route of changing the all task in the Makefile to include all necessary steps to complete a full build, including assets. It certainly caught me off guard that the suggested make generate build does not build assets.

So I'd suggest changing the all task to build everything, maybe including bindata as this seems to be the prefered build these days. Basically make make an alias to TAGS="bindata" make assets generate build (and probably other targets) and run just make on drone.

silverwind · 2017-06-03T08:42:32Z

Makefile

+		docker run --rm -v $(PWD):/app appleboy/node-less $< $@; \
+	else \
+		lessc $< $@; \
+	fi


It might be better to just depend on node and npm and then install node-less as a local dependency (with a package.json) than to depend on docker.

Not all golang developer have node environment so docker is better for everyone.

Okay, this is good for now. If we do a full node build for assets, I think adding it as a hard dependency might be best, it's usually just a apt-get away, no environment setup necessary.

I don't know what you mean. Can you explain the detail steps?

Modern frontend development includes a series of preprocessing to assets, like concatenating, automatically adding vendor prefixes, minifying css and js, precompiling gzip/brotli artifacts and more. The tools used for that all come from the Node.js/npm ecosystem, so I see Node.js as a requirement for any serious web development.

I think we can improve the workflow in dockerfile https://github.com/appleboy/node-less and make new PR to implement or maybe we can try the webpack solution.

In place of docker we could try https://github.com/kib357/less-go. It has a cli interface ready under : github.com/kib357/less-go/lessc. It is a little hacky implementation but it should work.

@sapk Good. I will try it out.

lunny · 2017-06-03T08:54:09Z

But that's not go getable?

appleboy · 2017-06-03T09:12:33Z

@lunny No. sad.

lunny · 2017-06-03T12:11:04Z

But I think go getable is a needed feature of Gitea.

bkcsoft · 2017-06-11T07:54:01Z

Makefile

@@ -35,12 +35,12 @@ else
 endif

 .PHONY: all
-all: build
+all: clean assets generate build


make shouldn't run clean 🙁

bkcsoft · 2017-06-11T07:54:57Z

Makefile

@@ -186,14 +186,18 @@ stylesheets: public/css/index.css

 .IGNORE: public/css/index.css
 public/css/index.css: $(STYLESHEETS)
-	lessc $< $@
+	@hash lessc > /dev/null 2>&1; if [ $$? -ne 0 ]; then \
+		docker run --rm -v $(PWD):/app appleboy/node-less $< $@; \


You never check that people have docker installed...

bkcsoft · 2017-06-11T07:56:45Z

Actually, don't do this, just add a make verify-css step 🙂

bkcsoft · 2017-06-11T08:03:08Z

$ git log --oneline --numstat $oldHead..$newHead | awk '/[0-9]+\s+[0-9]+/ { print $3 }' | grep ".less"
$ git log --oneline --numstat $oldHead..$newHead | awk '/[0-9]+\s+[0-9]+/ { print $3 }' | grep ".css"

Then check if both OR neither contain something

lunny · 2017-06-14T00:52:31Z

@appleboy agree with @bkcsoft add a verify on makefile is better.

appleboy · 2017-06-14T01:04:26Z

@lunny @bkcsoft I will update this PR.

bkcsoft · 2017-06-14T23:25:14Z

@appleboy FWIW, you need to check between base and compare branches otherwise this would happen

push less-file w/o css-file => build fails
push new commit w/o css/less-file => build passes

lunny · 2017-06-22T03:32:21Z

Let's move to v1.3

appleboy · 2017-07-11T07:30:00Z

Check output CSS file from less converter. Just like make fmt-check command.

sapk

Few little changes to keep historical commands working.

sapk · 2017-07-11T08:23:17Z

Makefile

+	fi;
+
+.PHONY: less
+less:


It would be better to update public/css/index.css and stylesheets target to have compat with old syntax and be included in assets

Maybe use stylesheets-check in place of less-check to be consistent but that would be a personal point and not mandatory. ^^

var STYLESHEETS if not needed anymore should be remove.

Solution is a little brutal but I like the cleanning. 😄

Why is this a .PHONY ? Why not public/css/index.css: ?

sapk · 2017-07-11T09:26:49Z

LGTM

appleboy · 2017-07-11T14:46:07Z

@bkcsoft Waiting for your response.

bkcsoft · 2017-07-12T01:32:36Z

@appleboy I don't like that it runs even when things aren't changed. See my comment in the review :)

Signed-off-by: Bo-Yi Wu <[email protected]>

appleboy · 2017-07-12T02:27:39Z

@bkcsoft Done. Please review again.

bkcsoft · 2017-07-12T02:28:20Z

LGTM

appleboy mentioned this pull request Jun 3, 2017

Prevent accidential selection of line numbers in code view #1860

Merged

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 3, 2017

appleboy added the type/enhancement An improvement of existing functionality label Jun 3, 2017

appleboy added this to the 1.2.0 milestone Jun 3, 2017

silverwind reviewed Jun 3, 2017

View reviewed changes

appleboy force-pushed the less branch from 25bacb8 to 4d50f8d Compare June 3, 2017 09:14

appleboy added the pr/wip This PR is not ready for review label Jun 4, 2017

bkcsoft suggested changes Jun 11, 2017

View reviewed changes

lunny modified the milestones: 1.3.0, 1.2.0 Jun 22, 2017

appleboy force-pushed the less branch from 4d50f8d to 4216993 Compare July 11, 2017 07:28

appleboy removed the pr/wip This PR is not ready for review label Jul 11, 2017

sapk suggested changes Jul 11, 2017

View reviewed changes

sapk approved these changes Jul 11, 2017

View reviewed changes

tboerger 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 Jul 11, 2017

appleboy added this to the 1.2.0 milestone Jul 11, 2017

appleboy removed this from the 1.3.0 milestone Jul 11, 2017

check less output file.

cf6e8d3

Signed-off-by: Bo-Yi Wu <[email protected]>

appleboy force-pushed the less branch from ac7e30f to cf6e8d3 Compare July 12, 2017 02:21

update command.

c8a22ed

Signed-off-by: Bo-Yi Wu <[email protected]>

tboerger 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 Jul 12, 2017

bkcsoft approved these changes Jul 12, 2017

View reviewed changes

bkcsoft merged commit 2b05b10 into go-gitea:master Jul 12, 2017

appleboy deleted the less branch July 12, 2017 02:29

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add convert less css file step. #1861

feat: add convert less css file step. #1861

appleboy commented Jun 3, 2017 •

edited

Loading

silverwind commented Jun 3, 2017 •

edited

Loading

silverwind Jun 3, 2017

appleboy Jun 3, 2017

silverwind Jun 3, 2017

appleboy Jun 3, 2017

silverwind Jun 3, 2017 •

edited

Loading

appleboy Jun 3, 2017 •

edited

Loading

sapk Jun 14, 2017

appleboy Jun 14, 2017

lunny commented Jun 3, 2017

appleboy commented Jun 3, 2017

lunny commented Jun 3, 2017

bkcsoft Jun 11, 2017

bkcsoft Jun 11, 2017

bkcsoft commented Jun 11, 2017

bkcsoft commented Jun 11, 2017

lunny commented Jun 14, 2017

appleboy commented Jun 14, 2017

bkcsoft commented Jun 14, 2017

lunny commented Jun 22, 2017

appleboy commented Jul 11, 2017

sapk left a comment

sapk Jul 11, 2017

sapk Jul 11, 2017

sapk Jul 11, 2017

appleboy Jul 11, 2017

sapk Jul 11, 2017

bkcsoft Jul 12, 2017

sapk commented Jul 11, 2017

appleboy commented Jul 11, 2017

bkcsoft commented Jul 12, 2017

appleboy commented Jul 12, 2017

bkcsoft commented Jul 12, 2017

feat: add convert less css file step. #1861

feat: add convert less css file step. #1861

Conversation

appleboy commented Jun 3, 2017 • edited Loading

silverwind commented Jun 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind Jun 3, 2017 • edited Loading

Choose a reason for hiding this comment

appleboy Jun 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Jun 3, 2017

appleboy commented Jun 3, 2017

lunny commented Jun 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkcsoft commented Jun 11, 2017

bkcsoft commented Jun 11, 2017

lunny commented Jun 14, 2017

appleboy commented Jun 14, 2017

bkcsoft commented Jun 14, 2017

lunny commented Jun 22, 2017

appleboy commented Jul 11, 2017

sapk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapk commented Jul 11, 2017

appleboy commented Jul 11, 2017

bkcsoft commented Jul 12, 2017

appleboy commented Jul 12, 2017

bkcsoft commented Jul 12, 2017

appleboy commented Jun 3, 2017 •

edited

Loading

silverwind commented Jun 3, 2017 •

edited

Loading

silverwind Jun 3, 2017 •

edited

Loading

appleboy Jun 3, 2017 •

edited

Loading