Add Octicon SVG spritemap #10107

jolheiser · 2020-02-02T06:51:07Z

To Do:

Initial pass, swap out octicons for SVGs
Go through and fix broken SVGs as noticed
Fix outside functionality (JS)

Signed-off-by: jolheiser <john.olheiser@gmail.com>

templates/repo/header.tmpl

guillep2k · 2020-02-02T14:47:19Z

make vendor?

jolheiser · 2020-02-02T15:15:38Z

This PR needs much more work before I look for approvals (sorry, just noticed I never marked as draft or WIP)

Currently I'm just gathering feedback on whether this seems like an acceptable solution. 😄

jolheiser · 2020-02-02T19:50:52Z

make vendor?

Interestingly, it's not picked up because of the build tag. 🤔

EDIT: Unless you were talking about the CI failure. 😅

Signed-off-by: jolheiser <john.olheiser@gmail.com>

scripts/generate-octicons.go

guillep2k

Could you add some comments at the top of the script file with instructions on how to update the icons? I'm guessing it's like:

Choose a new version of the library; update go.mod accordingly.
make vendor to bring it up to date.
Run the script with (insert exact go run command here).
git add, commit...

silverwind · 2020-02-02T22:53:10Z

Github does publish a npm module for octicons at https://www.npmjs.com/package/@primer/octicons. You could npm install --save @primer/octicons and take the SVGs from the node_modules/@primer/octicons/build/svg directory for the build.

jolheiser · 2020-02-02T23:07:41Z

@silverwind I was going to do that, but they don't publish a sprite, so we would need to still make one somehow, or use them individually.

While I'm sure it's possible, someone more experienced with JS would likely need to do it.

jolheiser · 2020-02-02T23:10:33Z

Yet another alternative is to forego the script altogether and manually stitch together a sprite whenever we want to update.

silverwind · 2020-02-02T23:10:46Z

The output file could be optimized further with tools like https://github.com/svg/svgo. I guess a make svg step could output to web_src/build (which is in .gitignore) and SVGO CLI could be used for the final optimization step (npx svg <input> <output>) and output to public.

As for spritesheeting, I have used https://github.com/svgstore/svgstore in the past, but there are probably better tools around.

templates/repo/header.tmpl

web_src/less/_svg.less

jolheiser · 2020-02-03T14:37:59Z

@silverwind I am using https://github.com/jkphl/svg-sprite, which apparently uses svgo in the output.

I will commit my changes once I have finished swapping out icons so that people can review. 😃

silverwind · 2020-02-03T17:31:00Z

@jolheiser ok. I see fill-rule and title in your output spritesheet. I think those could be eliminated by SVGO, they aren't really necessary.

Signed-off-by: jolheiser <john.olheiser@gmail.com>

jolheiser · 2020-02-05T03:36:03Z

I think this is more or less ready for review.
There were honestly far fewer mega-octicon than I expected, so if you'd rather I split up the template helper I can.
I considered not using a helper, but since now we need StaticUrlPrefix to access the sprite, I figured it would be easier to make the whole thing a helper.

I will definitely need feedback on the Makefile entry. I didn't include it in the webpack command since it's not likely to be modified often.
The library used to make the sprite claims to use svgo, but other than "minifying" I didn't see much difference.

And of course Drone is mad about PhantomJS, though I am unsure how exactly to resolve that...

modules/templates/helper.go

Signed-off-by: jolheiser <john.olheiser@gmail.com>

jolheiser · 2020-02-06T22:15:14Z

That last one seems to have fixed it. 🎉

webpack.config.js

Co-Authored-By: silverwind <me@silverwind.io>

Signed-off-by: jolheiser <john.olheiser@gmail.com>

…icon

Signed-off-by: jolheiser <john.olheiser@gmail.com>

web_src/js/index.js

silverwind · 2020-02-06T23:24:57Z

I think you still need to add a entry for icons.svg to templates/pwa/serviceworker_js.tmpl.

Signed-off-by: jolheiser <john.olheiser@gmail.com>

jolheiser · 2020-02-07T05:06:45Z

@silverwind Done with both. I didn't add the {{MD5 AppVer}} to the end, as I doubt the icons will change often.
But I'm also not an expert at the serviceworker, so let me know if I should add it.

silverwind · 2020-02-07T06:32:44Z

{{MD5 AppVer}} there is not a choice and only needed when the actual request has it which is not the case here.

One more related improvement that might be good to have is a preload entry for it, e.g.

<link rel="preload" as="image" href="{{StaticUrlPrefix}}/img/svg/icons.svg">

Signed-off-by: jolheiser <john.olheiser@gmail.com>

jolheiser · 2020-02-07T14:52:16Z

@silverwind Done.

silverwind · 2020-02-11T14:36:18Z

Any more reviews here? I'm very excited to get SVG icons in personally 😉

…icon

jolheiser · 2020-02-11T17:02:11Z

Ping lgtm

Add octicon SVG sprite

8000 5bd9a66

Signed-off-by: jolheiser <john.olheiser@gmail.com>

6543 reviewed Feb 2, 2020

View reviewed changes

templates/repo/header.tmpl Outdated Show resolved Hide resolved

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 2, 2020

jolheiser changed the title ~~Add Octicon SVG spritemap~~ WIP: Add Octicon SVG spritemap Feb 2, 2020

jolheiser added 3 commits February 2, 2020 13:52

Static prefix

e2249e7

Signed-off-by: jolheiser <john.olheiser@gmail.com>

SVG for all repo icons

7273e68

Signed-off-by: jolheiser <john.olheiser@gmail.com>

make vendor

215242b

Signed-off-by: jolheiser <john.olheiser@gmail.com>

guillep2k reviewed Feb 2, 2020

View reviewed changes

scripts/generate-octicons.go Outdated Show resolved Hide resolved

guillep2k reviewed Feb 2, 2020

View reviewed changes

silverwind reviewed Feb 2, 2020

View reviewed changes

templates/repo/header.tmpl Outdated Show resolved Hide resolved

silverwind reviewed Feb 2, 2020

View reviewed changes

web_src/less/_svg.less Show resolved Hide resolved

jolheiser added 3 commits February 3, 2020 23:39

Swap out octicons

d7d6e68

Signed-off-by: jolheiser <john.olheiser@gmail.com>

Move octicons to top of less imports

43b858a

Signed-off-by: jolheiser <john.olheiser@gmail.com>

Fix JS

6b717dc

Signed-off-by: jolheiser <john.olheiser@gmail.com>

techknowlogick reviewed Feb 5, 2020

View reviewed changes

modules/templates/helper.go Outdated Show resolved Hide resolved

techknowlogick added the topic/ui Change the appearance of the Gitea UI label Feb 5, 2020

techknowlogick added this to the 1.12.0 milestone Feb 5, 2020

jolheiser added 2 commits February 4, 2020 22:02

Definitely not a search/replace

85f29d1

Signed-off-by: jolheiser <john.olheiser@gmail.com>

Missed regex

e690697

Signed-off-by: jolheiser <john.olheiser@gmail.com>

silverwind reviewed Feb 6, 2020

View reviewed changes

webpack.config.js Outdated Show resolved Hide resolved

jolheiser and others added 5 commits February 6, 2020 16:29

Revert

0df9b40

Co-Authored-By: silverwind <me@silverwind.io>

Fix octions from upstream

8c319f2

Signed-off-by: jolheiser <john.olheiser@gmail.com>

Merge branch 'master' of https://github.com/go-gitea/gitea into octicon

89eb82c

Merge branch 'octicon' of https://github.com/jolheiser/gitea into oct…

6a37ddc

…icon

Fix Vue and missed JS function

bcf1ef8

Signed-off-by: jolheiser <john.olheiser@gmail.com>

silverwind reviewed Feb 6, 2020

View reviewed changes

web_src/js/index.js Outdated Show resolved Hide resolved

Add JS helper and PWA

8701f87

Signed-off-by: jolheiser <john.olheiser@gmail.com>

Preload SVG

4fa203e

Signed-off-by: jolheiser <john.olheiser@gmail.com>

jolheiser added 3 commits February 7, 2020 10:41

Merge branch 'master' into octicon

f184f24

Merge branch 'master' into octicon

5e5ae63

Merge branch 'master' into octicon

73f4eb9

jolheiser added 2 commits February 11, 2020 09:29

Merge branch 'master' of https://github.com/go-gitea/gitea into octicon

472a8bb

Merge branch 'octicon' of https://github.com/jolheiser/gitea into oct…

41a8a20

…icon

sapk approved these changes Feb 11, 2020

View reviewed changes

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, 2020

Merge branch 'master' into octicon

ec424cb

techknowlogick merged commit 86fdba1 into go-gitea:master Feb 11, 2020

jolheiser deleted the octicon branch February 11, 2020 17:03

jaqra mentioned this pull request Feb 11, 2020

Fix label menu text on miletone issues #10242

Merged

CirnoT mentioned this pull request May 14, 2020

Fix svg octicon-key placement #11410

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Add Octicon SVG spritemap #10107

Add Octicon SVG spritemap #10107

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Add Octicon SVG spritemap #10107

Add Octicon SVG spritemap #10107

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!