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 the possibility to allow the user to have a favicon which differs from the main logo #18542

Merged
merged 16 commits into from May 23, 2022

Conversation

je-s
Copy link
Contributor

@je-s je-s commented Feb 2, 2022

⚠️ BREAKING ⚠️

This change breaks users' current favicon configuration. Since we're replacing the path in the HTML file, Gitea will fall back to the standard Gitea logo for the favicon.
In order to fix this, users need to re-run the steps described in Customizing Gitea.
In short: The users need to copy their current logo.svg to favicon.svg, re-run make generate-images, and place the files in the folder accordingly as described in the guide linked above.

As described in #18541, this change allows the user to have a favicon which differs from his main logo.
In order to not break existing stuff, I've added/copied the icon.svg to favicon.svg. I've also adjusted the "Customizing Gitea" page already.

This allows the user to have a favicon which differs from the logo.
This is needed to accommodate the changes for allowing the user to have a differing logo and favicon
@techknowlogick techknowlogick changed the title Add the possibility to allow the user to have a favicon which differs from the main logo (#18541) Add the possibility to allow the user to have a favicon which differs from the main logo Feb 2, 2022
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 2, 2022
@silverwind
Copy link
Member

silverwind commented Feb 2, 2022

generate-images.js needs to be update to generate favicon.png from the favicon.svg source.

This is a breaking change for users of custom icons in any case because of the different HTTP filename.

@silverwind silverwind added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Feb 2, 2022
@silverwind silverwind added this to the 1.17.0 milestone Feb 2, 2022
@silverwind
Copy link
Member

silverwind commented Feb 2, 2022

Also, you need to alter generate-images.js so it actually copies the new file into public/img.

@je-s
Copy link
Contributor Author

je-s commented Feb 2, 2022

Hey @silverwind,

adjusted that, makes total sense. The changes are also tested and work as intended.

Regarding the breaking of currently customised favicons:
Would it make sense to pick that up as a pinned issue, so people know what to do as soon as they encounter it for the next few months? I mean, basically they just need to re-run the script and upload the files to their public folder.
And secondly, would it maybe make sense to make the setting/uploading of logos available via UI? (Like Nextcloud does it, for instance.) I'd make a separate issue for that then to discuss that matter.

@zeripath
Copy link
Contributor

zeripath commented Feb 2, 2022

as per the CONTRIBUTING.md:

If your PR could cause a breaking change you must add a BREAKING section to the opening comment e.g.:

## :warning: BREAKING :warning:

... explain how this is breaking and how to fix ...

To explain how this could affect users and how to mitigate these changes.

@je-s
Copy link
Contributor Author

je-s commented Feb 3, 2022

I've added a clearer description of how generate-images works now.

@stale
Copy link

stale bot commented Apr 16, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Apr 16, 2022
@je-s
Copy link
Contributor Author

je-s commented May 10, 2022

Is there anything left to be done to get it included into 1.17.0?
If so I'll definitely do it. I've seen that it's currently stuck at being reviewed by two people.

@stale stale bot removed the issue/stale label May 10, 2022
@GiteaBot 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 May 10, 2022
@silverwind
Copy link
Member

Maybe update the PR description once more with instructions for users that had used the previous single source file, e.g. that they now have to copy their logo.svg to favicon.svg to achieve the previous outcome.

@GiteaBot GiteaBot removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 11, 2022
@GiteaBot GiteaBot added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label May 11, 2022
@lunny
Copy link
Member

lunny commented May 11, 2022

Please resolve the conflicts

@je-s
Copy link
Contributor Author

je-s commented May 11, 2022

@lunny Done, conflicts resolved.

@techknowlogick
Copy link
Member

Thanks @je-s, the linting is failing on the head.tmpl due to mix use of tabs v spaces. If you have time to send a new commit to resolve I can merge now, otherwise I can update your branch later with the fix.

@je-s
Copy link
Contributor Author

je-s commented May 11, 2022

@techknowlogick Should be fine now, done.
@silverwind I've added a short description regarding how to solve that issue, additionally to the link where the (updated) guide can be found. I guess if we're linking/hinting that in the release notes, it should be clear enough.

@silverwind
Copy link
Member

silverwind commented May 11, 2022

Something is wrong with your merge, the window.config section should not be in templates/base/head.tmpl, it was extracted to a separate file a while ago. Don' merge until resolved.

Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge needs to be fixed.

Copy link
Member

@lafriks lafriks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking until failed merge is fixed

@je-s je-s requested review from lafriks and silverwind May 11, 2022 23:47
@je-s
Copy link
Contributor Author

je-s commented May 11, 2022

@lafriks @silverwind Yeah, this old section got falsely included. I've removed the window.config script section to reflect the current state again. I think we should be good now finally. :)

@lunny lunny dismissed lafriks’s stale review May 12, 2022 07:07

failed merge is fixed

@wxiaoguang
Copy link
Contributor

If there is nothing more left to do, this PR will be merged in 2 days (some more open days, in case some people want to improve it).

@lunny lunny merged commit b65ad70 into go-gitea:main May 23, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request May 24, 2022
* giteaofficial/main:
  [skip ci] Updated translations via Crowdin
  Add the possibility to allow the user to have a favicon which differs from the main logo (go-gitea#18542)
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
… from the main logo (go-gitea#18542)

* Changed the filename of the favicon SVG

This allows the user to have a favicon which differs from the logo.

* Added favicon.svg

This is needed to accommodate the changes for allowing the user to have a differing logo and favicon

* Adjusted page to accommodate what icon is used as favicon

* Added functionality to also generate the favicon.svg via generate-images.js

* Adjusted the description for the new favicon compatibility

Co-authored-by: silverwind <me@silverwind.io>

* Updated generate-images.js to generate favicons from a separate favicons.svg file

This belongs to PR go-gitea#18542.

* Added description on how custom favicons can be generated

* Replaced space indents with tabs

* Synced changes with current state of the file

* Synced changes with current state of the file

Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: zeripath <art27@cantab.net>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: Lauris BH <lauris@nix.lv>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants