The Wayback Machine - https://web.archive.org/web/20210807164255/https://github.com/go-gitea/gitea/pull/13680
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

Clean up SVG #13680

Merged
merged 15 commits into from Dec 17, 2020
Merged

Clean up SVG #13680

merged 15 commits into from Dec 17, 2020

Conversation

@techknowlogick
Copy link
Member

@techknowlogick techknowlogick commented Nov 23, 2020

The previous SVG was a combination of objects and paths. This PR cleans up the SVG.

@silverwind
Copy link
Member

@silverwind silverwind commented Nov 23, 2020

That inner border did look kind of nice on big logos, but I guess flat is the trend. Is the color change intentional?

Also, the new SVG does not include the detail-remove class anymore and I generally think we can drop that mechanism and you should apply this patch to do so:

Patch
diff --git a/Makefile b/Makefile
index 3c1304575..d5b707372 100644
--- a/Makefile
+++ b/Makefile
@@ -686,9 +686,9 @@ generate-gitignore:


 .PHONY: generate-images
 generate-images:
-	npm install --no-save --no-package-lock xmldom fabric imagemin-zopfli
+	npm install --no-save --no-package-lock fabric imagemin-zopfli
 	node build/generate-images.js

 .PHONY: pr\#%
 pr\#%: clean-all
diff --git a/build/generate-images.js b/build/generate-images.js
index 9b7b82017..c7f58f61d 100755
--- a/build/generate-images.js
+++ b/build/generate-images.js
@@ -2,9 +2,8 @@
 'use strict';

 const imageminZopfli = require('imagemin-zopfli');
 const {fabric} = require('fabric');
-const {DOMParser, XMLSerializer} = require('xmldom');
 const {readFile, writeFile} = require('fs').promises;
 const {resolve} = require('path');
 const Svgo = require('svgo');

@@ -39,25 +38,9 @@ async function generateSvgFavicon(svg, outputFile) {
   const {data} = await svgo.optimize(svg);
   await writeFile(outputFile, data);
 }

-async function generate(svg, outputFile, {size, bg, removeDetail} = {}) {
-  const parser = new DOMParser();
-  const serializer = new XMLSerializer();
-  const document = parser.parseFromString(svg);
-
-  if (removeDetail) {
-    for (const el of Array.from(document.getElementsByTagName('g') || [])) {
-      for (const attribute of Array.from(el.attributes || [])) {
-        if (attribute.name === 'class' && attribute.value === 'detail-remove') {
-          el.parentNode.removeChild(el);
-        }
-      }
-    }
-  }
-
-  svg = serializer.serializeToString(document);
-
+async function generate(svg, outputFile, {size, bg}) {
   const {objects, options} = await loadSvg(svg);
   const canvas = new fabric.Canvas();
   canvas.setDimensions({width: size, height: size});
   const ctx = canvas.getContext('2d');
@@ -92,9 +75,9 @@ async function main() {
   await generate(svg, resolve(__dirname, '../public/img/gitea-512.png'), {size: 512});
   await generate(svg, resolve(__dirname, '../public/img/gitea-192.png'), {size: 192});
   await generate(svg, resolve(__dirname, '../public/img/gitea-sm.png'), {size: 120});
   await generate(svg, resolve(__dirname, '../public/img/avatar_default.png'), {size: 200});
-  await generate(svg, resolve(__dirname, '../public/img/favicon.png'), {size: 180, removeDetail: true});
+  await generate(svg, resolve(__dirname, '../public/img/favicon.png'), {size: 180});
   await generate(svg, resolve(__dirname, '../public/img/apple-touch-icon.png'), {size: 180, bg: true});
 }

 main().then(exit).catch(exit);

Also, I guess something needs to be done about loading.png. Maybe convert it to a SVG that pulses on load or something else simple?

Copy link
Member

@6543 6543 left a comment

the new color is brighter - should we realy change it?

@silverwind
Copy link
Member

@silverwind silverwind commented Nov 24, 2020

I'm fine with the color change personally. The border removal too as is not really noticable except on a fullscreen logo anyways.

.st0{fill:#FFFFFF;}
.st1{fill:#73A952;}
</style>

This comment has been minimized.

@silverwind

silverwind Nov 24, 2020
Member

Can you move those fills to the path elements as fill attribute and then remove class on them?

@lunny
Copy link
Member

@lunny lunny commented Nov 25, 2020

@techknowlogick Could you paste some screenshots?

@silverwind
Copy link
Member

@silverwind silverwind commented Nov 25, 2020

@lunny click "Display Rich Diff" on the images in the diff.

@silverwind
Copy link
Member

@silverwind silverwind commented Nov 25, 2020

Looking at the diff once more, I can't help but feel the new logo is a downgrade. I think the new color looks a bit dull compared to the older more vibrant one and the removal of the border makes it seem lower quality as well to me.

Also, the "zoom in" will cause issues in systems like Discord that force a circle clipping of the image, I'd say we even need to zoom out the old logo a tiny bit further to prevent such clipping.

image

@techknowlogick
Copy link
Member Author

@techknowlogick techknowlogick commented Dec 2, 2020

For those who were unsure about the colour change, I've changed the colour back to the original. As for things such as discord clipping we can generate an image that that has appropriate image dimensions as different sites require different dimension requirements.

Screen Shot 2020-12-01 at 9 54 33 PM

@codecov-io
Copy link

@codecov-io codecov-io commented Dec 2, 2020

Codecov Report

Merging #13680 (81e7454) into master (c3fc190) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #13680   +/-   ##
=======================================
  Coverage   42.23%   42.23%           
=======================================
  Files         710      710           
  Lines       77233    77233           
=======================================
+ Hits        32617    32623    +6     
  Misses      39245    39245           
+ Partials     5371     5365    -6     
Impacted Files Coverage Δ
modules/git/utils.go 73.77% <0.00%> (-3.28%) ⬇️
modules/process/manager.go 72.50% <0.00%> (-2.50%) ⬇️
modules/git/blame.go 65.71% <0.00%> (-1.43%) ⬇️
services/mailer/mail.go 60.21% <0.00%> (-1.08%) ⬇️
modules/git/repo.go 45.72% <0.00%> (-0.51%) ⬇️
models/issue_comment.go 52.71% <0.00%> (-0.16%) ⬇️
modules/notification/mail/mail.go 39.08% <0.00%> (ø)
models/error.go 39.31% <0.00%> (+0.32%) ⬆️
services/pull/check.go 51.82% <0.00%> (+0.72%) ⬆️
models/notification.go 67.24% <0.00%> (+0.98%) ⬆️
... and 3 more

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 c3fc190...81e7454. Read the comment docs.

@lunny
Copy link
Member

@lunny lunny commented Dec 2, 2020

The new logo looks bigger than the old one and lack of border.

@silverwind
Copy link
Member

@silverwind silverwind commented Dec 2, 2020

Why not zoom out the SVG so it fits in a circle around the outer edges? I guess the difference would be hardly noticeable compared to current version.

@techknowlogick
Copy link
Member Author

@techknowlogick techknowlogick commented Dec 5, 2020

After having to edit a unique avatar sizing for discord, twitter, reddit, and github, I can tell you two things 1. I reaally dislike forcing circle avatars on users, and 2. by having the SVG as we have it now, allows us to scale the logo as needed, and not worry about extra spacing when calculating things.

@silverwind
Copy link
Member

@silverwind silverwind commented Dec 5, 2020

Hmm yeah I guess we want maximum available area, for things like favicon.

@kdumontnu kdumontnu mentioned this pull request Dec 14, 2020
@6543
6543 approved these changes Dec 15, 2020
@kdumontnu
Copy link
Contributor

@kdumontnu kdumontnu commented Dec 17, 2020

@techknowlogick @silverwind is this PR still active/under review? It's blocking my PR. Anything I can do to help?

@silverwind
Copy link
Member

@silverwind silverwind commented Dec 17, 2020

No, seems good now. I didn't see it was ready.

@techknowlogick techknowlogick merged commit f8a668a into go-gitea:master Dec 17, 2020
1 check was pending
1 check was pending
continuous-integration/drone/pr Build is running
Details
@techknowlogick techknowlogick deleted the techknowlogick:update-logo-svg branch Dec 17, 2020
silverwind added a commit to silverwind/gitea that referenced this pull request Dec 17, 2020
The xmldom dependency is no longer required since go-gitea#13680. Also,
whitespace cleanup.
@silverwind silverwind mentioned this pull request Dec 17, 2020
6543 pushed a commit that referenced this pull request Dec 17, 2020
* Makefile cleanup

The xmldom dependency is no longer required since #13680. Also,
whitespace cleanup.

* double the golangci-lint timeout
@go-gitea go-gitea locked and limited conversation to collaborators Jan 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants