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 Octicon SVG spritemap #10107

Merged
merged 38 commits into from Feb 11, 2020
Merged

Add Octicon SVG spritemap #10107

merged 38 commits into from Feb 11, 2020

Conversation

@jolheiser
Copy link
Member

jolheiser commented Feb 2, 2020

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 Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 label Feb 2, 2020
@guillep2k

This comment has been minimized.

Copy link
Member

guillep2k commented Feb 2, 2020

make vendor?

@jolheiser

This comment has been minimized.

Copy link
Member Author

jolheiser commented Feb 2, 2020

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 jolheiser changed the title Add Octicon SVG spritemap WIP: Add Octicon SVG spritemap Feb 2, 2020
@jolheiser

This comment has been minimized.

Copy link
Member Author

jolheiser commented Feb 2, 2020

make vendor?

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

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

jolheiser added 3 commits Feb 2, 2020
Signed-off-by: jolheiser <john.olheiser@gmail.com>
Signed-off-by: jolheiser <john.olheiser@gmail.com>
Signed-off-by: jolheiser <john.olheiser@gmail.com>
scripts/generate-octicons.go Outdated Show resolved Hide resolved
Copy link
Member

guillep2k left a comment

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

This comment has been minimized.

Copy link
Member

silverwind commented Feb 2, 2020

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

This comment has been minimized.

Copy link
Member Author

jolheiser commented Feb 2, 2020

@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

This comment has been minimized.

Copy link
Member Author

jolheiser commented Feb 2, 2020

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

@silverwind

This comment has been minimized.

Copy link
Member

silverwind commented Feb 2, 2020

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 Outdated Show resolved Hide resolved
@jolheiser

This comment has been minimized.

Copy link
Member Author

jolheiser commented Feb 3, 2020

@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

This comment has been minimized.

Copy link
Member

silverwind commented Feb 3, 2020

@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.

jolheiser added 3 commits Feb 4, 2020
Signed-off-by: jolheiser <john.olheiser@gmail.com>
Signed-off-by: jolheiser <john.olheiser@gmail.com>
Fix JS
Signed-off-by: jolheiser <john.olheiser@gmail.com>
@jolheiser jolheiser changed the title WIP: Add Octicon SVG spritemap Add Octicon SVG spritemap Feb 5, 2020
@jolheiser

This comment has been minimized.

Copy link
Member Author

jolheiser commented Feb 5, 2020

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...

@techknowlogick techknowlogick added this to the 1.12.0 milestone Feb 5, 2020
jolheiser added 2 commits Feb 5, 2020
Signed-off-by: jolheiser <john.olheiser@gmail.com>
Signed-off-by: jolheiser <john.olheiser@gmail.com>
Signed-off-by: jolheiser <john.olheiser@gmail.com>
@jolheiser

This comment has been minimized.

Copy link
Member Author

jolheiser commented Feb 6, 2020

That last one seems to have fixed it. 🎉

webpack.config.js Outdated Show resolved Hide resolved
jolheiser and others added 5 commits Feb 6, 2020
Revert
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 Outdated Show resolved Hide resolved
@silverwind

This comment has been minimized.

Copy link
Member

silverwind commented Feb 6, 2020

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

This comment has been minimized.

Copy link
Member Author

jolheiser commented Feb 7, 2020

@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

This comment has been minimized.

Copy link
Member

silverwind commented Feb 7, 2020

{{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

This comment has been minimized.

Copy link
Member Author

jolheiser commented Feb 7, 2020

@silverwind Done.

jolheiser added 3 commits Feb 7, 2020
@silverwind

This comment has been minimized.

Copy link
Member

silverwind commented Feb 11, 2020

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

jolheiser added 2 commits Feb 11, 2020
…icon
@sapk
sapk approved these changes Feb 11, 2020
@GiteaBot GiteaBot added lgtm/done and removed lgtm/need 1 labels Feb 11, 2020
@jolheiser

This comment has been minimized.

Copy link
Member Author

jolheiser commented Feb 11, 2020

Ping lgtm

@techknowlogick techknowlogick merged commit 86fdba1 into go-gitea:master Feb 11, 2020
2 checks passed
2 checks passed
approvals/lgtm this commit looks good
continuous-integration/drone/pr Build is passing
Details
@jolheiser jolheiser deleted the jolheiser:octicon branch Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.