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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Inject SVG sprite via ajax #10320

Merged
merged 6 commits into from
Feb 17, 2020
Merged

Inject SVG sprite via ajax #10320

merged 6 commits into from
Feb 17, 2020

Conversation

jolheiser
Copy link
Member

Per discussion in Discord, gitea.com currently has no SVG icons. 馃槺

https://stackoverflow.com/questions/27458729/amazon-s3-cors-issue-with-svg-on-all-major-browser
https://css-tricks.com/ajaxing-svg-sprite/

This PR uses the method mentioned in the linked post.

Ping @silverwind for feedback or suggestions concerning this method and injecting the sprite into the DOM.

@jolheiser
AJAX SVG
4375133 
Signed-off-by: jolheiser <[email protected]>
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Feb 17, 2020
@zeripath zeripath added type/bug topic/ui Change the appearance of the Gitea UI labels Feb 17, 2020
@zeripath zeripath added this to the 1.12.0 milestone Feb 17, 2020
@silverwind
Copy link
Member

Can we inline the content of /img/svg/icons.svg into head.tmpl right after <body>? That'd be my preferred solution as it both circumvents any CORS issues as well as makes the icons immediately available.

@jolheiser
Copy link
Member Author

According to the article, this method preserved being able to cache it whereas inlining wouldn't?

@silverwind
Copy link
Member

Yeah, it would not be able to cache it and it would add 70kB to every HTML request, so probably not the way to go. Ideally, the <use> element would expose a crossorigin attribute but apparently no browser has implemented such a feature. Testing this right now.

@silverwind
Copy link
Member

silverwind commented Feb 17, 2020 •
edited

There is a syntax error in templates/pwa/serviceworker_js.tmpl where the entry '{{StaticUrlPrefix}}/img/svg/icons.svg' is missing a trailing comma, can you fix that here?

@jolheiser
Fix PWA
d73f55d 
Signed-off-by: jolheiser <[email protected]>
@jolheiser
Copy link
Member Author

Added the comma

@silverwind
Copy link
Member

Oh, and please remove serviceworker entries for '{{StaticUrlPrefix}}/vendor/assets/octicons/octicons.min.css', and '{{StaticUrlPrefix}}/vendor/assets/octicons/octicons.woff2?ef21c39f0ca9b1b5116e5eb7ac5eabe6',, those files no longer exist.

@codecov-io
Copy link

Codecov Report

Merging #10320 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #10320   +/-   ##
=======================================
  Coverage   43.72%   43.72%           
=======================================
  Files         585      585           
  Lines       81015    81015           
=======================================
  Hits        35421    35421           
  Misses      41211    41211           
  Partials     4383     4383

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 4375133...d73f55d. Read the comment docs.

@jolheiser
Remove unused PWA assets
9a4ac98 
Signed-off-by: jolheiser <[email protected]>
@silverwind
Copy link
Member

SW working again after those fixes.

@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 17, 2020
silverwind
silverwind reviewed Feb 17, 2020
View reviewed changes
web_src/js/index.js Outdated Show resolved Hide resolved
@jolheiser
Copy link
Member Author

Thanks for the quick response and help! 馃

@jolheiser @silverwind
Update JS comment
4ee2632 
Co-Authored-By: silverwind <[email protected]>
@silverwind
Copy link
Member

I wasn't aware of this issue with <use> not being able to CORS but it's good we have gitea.com to test CORS 馃槅.

@zeripath
Copy link
Contributor

Make lg-tm work

@zeripath zeripath merged commit e76a64d into go-gitea:master Feb 17, 2020
@jolheiser jolheiser mentioned this pull request Feb 18, 2020
Merged
@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.
Reviewers

@silverwind silverwind silverwind approved these changes

@zeripath zeripath zeripath approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI type/bug
Projects
None yet
Milestone
1.12.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jolheiser @silverwind @codecov-io @zeripath @GiteaBot