gitea png to logo #13974

kdumontnu · 2020-12-13T18:59:54Z

More general usage for logo png files to make it easier for users to customize logo.

Users should now be able to replace the file logo.svg with a custom svg logo and run make generate-images to update the logo (no need to change the templates).

Note: I've kept gitea.svg and gitea-192.png for use in locations that should retain the gitea logo even after the fork logo is updated, such as the README files and webhook html templates. To regenerate these images, run TAGS="gitea" make generate-images.

Edit: After some discussion (see below), I've also updated most html template references to logo.svg instead of png.

codecov-io · 2020-12-13T19:40:53Z

Codecov Report

❗ No coverage uploaded for pull request base (master@e25e7b9). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #13974   +/-   ##
=========================================
  Coverage          ?   42.26%           
=========================================
  Files             ?      726           
  Lines             ?    77684           
  Branches          ?        0           
=========================================
  Hits              ?    32830           
  Misses            ?    39479           
  Partials          ?     5375

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 e25e7b9...dac624a. Read the comment docs.

6543 · 2020-12-14T10:18:20Z

It's kind breaking since lot of sites have customized there logo and have to change the path if merged ...

lunny

I don't think this is necessary. You can change the customized template to use your own logo.

kdumontnu · 2020-12-14T16:03:54Z

@6543 Yes, this is likely a breaking change

@lunny This simplifies the process and reduces the number of upstream conflicts in new forked projects. The idea here is twofold:

Users don't need to modify each template that references a logo file. They can now just replace the logo pngs or simply change assets/logo.svg and run make generate-images.
It keeps a separate gitea logo file for places in template that should keep the gitea image even when the main logo is updated (ex. webhooks)

I'll leave it up to you and the rest of the maintainers whether a potentially breaking change for existing forks is worth it to reduce the number of breaking changes in future forks. I think so, but happy to close the PR if the team sees otherwise 👍

techknowlogick

Thanks for PR :)

Blocking until #13680 is merged due to potential merge conflicts that will arise from both PRs.

kdumontnu · 2020-12-14T19:42:22Z

Thanks for PR :)

Blocking until #13680 is merged due to potential merge conflicts that will arise from both PRs.

K, I can rebase when that gets merged.

One solution to avoid the breaking change is to keep the gitea-X.png files as they are, change the generate-images script and templates to logo-X.png, add generate-images to make build, but don't commit the logo-X.png files (not sure why we would commit these anyways).

This way, anyone referencing gitea-X.png will still have access. It's a bit more involved change and would likely require a new CI test though.

silverwind · 2020-12-14T22:13:30Z

Rename is a good idea but I wonder if we can cut down the amount of different sizes. We could use SVG in HTML exclusively and it would be consistent with the favicon which we requires as SVG already.

It would get harder to use raster images for customization but apparently it's not impossible to embed them in SVG and with the favicon already changed, we have already committed to requiring a vector image.

kdumontnu · 2020-12-14T22:37:55Z

Rename is a good idea but I wonder if we can cut down the amount of different sizes. We could use SVG in HTML exclusively and it would be consistent with the favicon which we requires as SVG already.

It would get harder to use raster images for customization but apparently it's not impossible to embed them in SVG and with the favicon already changed, we have already committed to requiring a vector image.

+1 to svg, I'll likely use svg in my fork. If this is the accepted solution I'm happy to make this change. I would leave gitea-sm.png and gitea-lg.png so that we can reference them in the README (until gitea supports svg 😄). Unless I hear otherwise, this is the direction I'll take it (@techknowlogick).

The only place 512 and 192 are referenced is pwa/manifest_json.tmpl, which I know very little about. Although apparently they support svgs with "sizes": "any".
edit: except on Chrome 🙄 well, I think we should be able to still use "sizes": "48x48 72x72 96x96 128x128 256x256"

silverwind · 2020-12-14T23:10:41Z

Full conversation to SVG will need more consideration and testing, I guess we can do it another time. I think the primary blockers are mobile browsers which use the PNGs for the homescreen icon. I can't find any definitive info on SVG support for those but I'd expect some issues.

For example Chrome devtools can not deal with any:

https://bugs.chromium.org/p/chromium/issues/detail?id=1107123

kdumontnu · 2020-12-14T23:27:11Z

Full conversation to SVG will need more consideration and testing, I guess we can do it another time. I think the primary blockers are mobile browsers which use the PNGs for the homescreen icon. I can't find any definitive info on SVG support for those but I'd expect some issues.

For example Chrome devtools can not deal with any:

https://bugs.chromium.org/p/chromium/issues/detail?id=1107123

I can at least update all of the template html references to svg and leave the manifest alone for now. It resolves my concern and removes a future breaking change.

silverwind · 2020-12-15T00:02:17Z

Yeah, HTML is always fine to update.

silverwind · 2020-12-15T00:19:49Z

I think fluid-icon can also be removed if we can confirm that Firefox will use the SVG favicon.

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/UI_considerations#How_an_icon_is_selected

techknowlogick

Dismissing my blocking review.

Thanks for this PR :)

templates/base/head.tmpl

kdumontnu · 2020-12-18T05:21:25Z

Dismissing my blocking review.

Thanks for this PR :)

@techknowlogick You're very welcome. I've made some significant changes and rebased since you approved - can you take another look?

lunny · 2020-12-18T16:13:11Z

CI failed seems unrelated.

public/img/gitea.svg

Makefile

kdumontnu · 2020-12-18T22:51:18Z

This PR should be all set to merge 👍

techknowlogick · 2020-12-19T01:17:38Z

Thanks again :)

silverwind · 2020-12-19T02:37:23Z

As a next step, I think we should reduce the logos in use further, there is no reason to fetch logo.svg and favicon.svg, they are essentially the same resource. We can combine them into a 32px SVG logo.svg

Dimensions shouldn't matter for a SVG file but I recall keeping it at 32px for possible compatibilty for favicons.

Also, we can possible cut down on the number of PNGs.

kdumontnu · 2020-12-24T05:16:24Z

Good read-up here: https://evilmartians.com/chronicles/how-to-favicon-in-2021-six-files-that-fit-most-needs

silverwind · 2020-12-24T09:52:12Z

Yeah, seen it. I disagree about the .ico thought, it's just unnecessary, PNG/SVG covers everything.

Followup PR: #14136

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

6543 added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Dec 14, 2020

6543 added this to the 1.14.0 milestone Dec 14, 2020

6543 added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Dec 14, 2020

zeripath approved these changes Dec 14, 2020

View reviewed changes

GiteaBot 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 Dec 14, 2020

lunny requested changes Dec 14, 2020

View reviewed changes

techknowlogick requested changes Dec 14, 2020

View reviewed changes

techknowlogick approved these changes Dec 18, 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 Dec 18, 2020

lunny reviewed Dec 18, 2020

View reviewed changes

templates/base/head.tmpl Outdated Show resolved Hide resolved

gitea png to logo.svg

7623ac0

kdumontnu force-pushed the kd/standard_logo_name branch from 82a81b9 to 7623ac0 Compare December 18, 2020 05:01

kdumontnu added 2 commits December 18, 2020 00:02

Merge branch 'master' into kd/standard_logo_name

33e00d7

gitea safari svg to logo

bbc28cb

kdumontnu requested review from zeripath and techknowlogick December 18, 2020 05:28

kdumontnu requested a review from lunny December 18, 2020 05:28

lunny approved these changes Dec 18, 2020

View reviewed changes

silverwind reviewed Dec 18, 2020

View reviewed changes

public/img/gitea.svg Outdated Show resolved Hide resolved

silverwind reviewed Dec 18, 2020

View reviewed changes

Makefile Show resolved Hide resolved

kdumontnu added 4 commits December 18, 2020 13:28

minify svgs

7d50b8a

Update english docs

3407f6a

Merge branch 'master' into kd/standard_logo_name

0cecaef

Update missing section on customizing logo

dac624a

silverwind approved these changes Dec 18, 2020

View reviewed changes

techknowlogick approved these changes Dec 19, 2020

View reviewed changes

techknowlogick merged commit 4cd94e3 into go-gitea:master Dec 19, 2020

kdumontnu deleted the kd/standard_logo_name branch December 27, 2020 18:13

go-gitea locked and limited conversation to collaborators Feb 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gitea png to logo #13974

gitea png to logo #13974

kdumontnu commented Dec 13, 2020 •

edited

Loading

codecov-io commented Dec 13, 2020 •

edited

Loading

6543 commented Dec 14, 2020

lunny left a comment

kdumontnu commented Dec 14, 2020

techknowlogick left a comment

kdumontnu commented Dec 14, 2020

silverwind commented Dec 14, 2020

kdumontnu commented Dec 14, 2020 •

edited

Loading

silverwind commented Dec 14, 2020 •

edited

Loading

kdumontnu commented Dec 14, 2020

silverwind commented Dec 15, 2020

silverwind commented Dec 15, 2020

techknowlogick left a comment

kdumontnu commented Dec 18, 2020

lunny commented Dec 18, 2020

kdumontnu commented Dec 18, 2020

techknowlogick commented Dec 19, 2020

silverwind commented Dec 19, 2020 •

edited

Loading

kdumontnu commented Dec 24, 2020

silverwind commented Dec 24, 2020 •

edited

Loading

gitea png to logo #13974

gitea png to logo #13974

Conversation

kdumontnu commented Dec 13, 2020 • edited Loading

codecov-io commented Dec 13, 2020 • edited Loading

Codecov Report

6543 commented Dec 14, 2020

lunny left a comment

Choose a reason for hiding this comment

kdumontnu commented Dec 14, 2020

techknowlogick left a comment

Choose a reason for hiding this comment

kdumontnu commented Dec 14, 2020

silverwind commented Dec 14, 2020

kdumontnu commented Dec 14, 2020 • edited Loading

silverwind commented Dec 14, 2020 • edited Loading

kdumontnu commented Dec 14, 2020

silverwind commented Dec 15, 2020

silverwind commented Dec 15, 2020

techknowlogick left a comment

Choose a reason for hiding this comment

kdumontnu commented Dec 18, 2020

lunny commented Dec 18, 2020

kdumontnu commented Dec 18, 2020

techknowlogick commented Dec 19, 2020

silverwind commented Dec 19, 2020 • edited Loading

kdumontnu commented Dec 24, 2020

silverwind commented Dec 24, 2020 • edited Loading

kdumontnu commented Dec 13, 2020 •

edited

Loading

codecov-io commented Dec 13, 2020 •

edited

Loading

kdumontnu commented Dec 14, 2020 •

edited

Loading

silverwind commented Dec 14, 2020 •

edited

Loading

silverwind commented Dec 19, 2020 •

edited

Loading

silverwind commented Dec 24, 2020 •

edited

Loading