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 touch-icon with background #10022

Merged
merged 5 commits into from
Jan 29, 2020
Merged

Conversation

NiTRoeSE
Copy link
Contributor

To prevent this ugly glitch while minimize app to homescreen because of transparent background of favicon.

IMG_0329

You MUST delete the content above including this line before posting, otherwise your pull request will be invalid.

...to prevent ugly glitch while minimize app to homescreen
@codecov-io
Copy link

codecov-io commented Jan 27, 2020

Codecov Report

Merging #10022 into master will increase coverage by <.01%.
The diff coverage is 41.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10022      +/-   ##
==========================================
+ Coverage   42.26%   42.27%   +<.01%     
==========================================
  Files         611      616       +5     
  Lines       80407    80742     +335     
==========================================
+ Hits        33983    34132     +149     
- Misses      42244    42410     +166     
- Partials     4180     4200      +20
Impacted Files Coverage Δ
modules/queue/queue.go 26.82% <0%> (-8.47%) ⬇️
routers/admin/admin.go 10.64% <0%> (-0.57%) ⬇️
modules/migrations/gitea.go 6.14% <0%> (ø) ⬆️
routers/private/manager_unix.go 0% <0%> (ø)
services/pull/review.go 0% <0%> (ø) ⬆️
cmd/manager.go 0% <0%> (ø)
routers/repo/commit.go 29.66% <0%> (ø) ⬆️
routers/private/manager.go 0% <0%> (ø)
modules/graceful/manager_unix.go 40.83% <0%> (-2.53%) ⬇️
modules/queue/queue_redis.go 1.48% <0%> (+0.06%) ⬆️
... and 43 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 f718b50...4946b72. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 27, 2020
@silverwind
Copy link
Member

Those images come from this make task, so it'd need to be added there. As for adding the background, you could try something like convert favicon.png -fill white -opaque none apple-touch-icon.png.

@techknowlogick techknowlogick added this to the 1.12.0 milestone Jan 27, 2020
@techknowlogick techknowlogick added type/enhancement An improvement of existing functionality topic/ui Change the appearance of the Gitea UI labels Jan 27, 2020
@NiTRoeSE
Copy link
Contributor Author

Those images come from this make task, so it'd need to be added there. As for adding the background, you could try something like convert favicon.png -fill white -opaque none apple-touch-icon.png.

.. thanks for the hint, in fact i have no clue from go but wanted to help to fix this little cosmetic problem. ☺️ ..I hope its now in a correct way, if not i think its better that someone which have the needed knowledge do this job. regards 🙂

@techknowlogick
Copy link
Member

in fact i have no clue from go but wanted to help to fix this little cosmetic problem

no worries. thanks for this PR :) I think one last thing that needs to be done is if you run the make task and then commit the generated image from that

@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 Jan 28, 2020
@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 Jan 28, 2020
@lafriks
Copy link
Member

lafriks commented Jan 28, 2020

Generated image still needs to be included in conmit

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.

Generated icon missing

@techknowlogick techknowlogick dismissed lafriks’s stale review January 29, 2020 01:26

I've run make command

@techknowlogick techknowlogick added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Jan 29, 2020
@techknowlogick techknowlogick merged commit db42a15 into go-gitea:master Jan 29, 2020
@silverwind
Copy link
Member

silverwind commented Jan 29, 2020

The image still has some transparent pixels left, making it look bad on a dark background:

I'd say revert this commit. If IOS can not deal with transparency, it's IOS's fault, not ours. I certainly prefer transparency over a opinionated white background. It certainly does not look out of place to me in the animation in the OP.

silverwind added a commit to silverwind/gitea that referenced this pull request Jan 29, 2020
@NiTRoeSE
Copy link
Contributor Author

The image still has some transparent pixels left, making it look bad on a dark background:

I'd say revert this commit. If IOS can not deal with transparency, it's IOS's fault, not ours. I certainly prefer transparency over a opinionated white background. It certainly does not look out of place to me in the animation in the OP.

I ran make yesterday in the the evening, i needed more time to figure out how to do it because its my first time with go. As far i could see yesterday there was no transparency left and the icon looked good. I take a look again this evening and give feedback here..

Except of this .. why not just add an photoshop generated icon for this case ? (I could create one...) Or in other words, why must it be generated through makefile?

@6543
Copy link
Member

6543 commented Jan 29, 2020

@NiTRoeSE it doesnt have to ... For me generate-images looks like a relict from the past ...
@techknowlogick any opinion of removing generate-images or just leave it as it is ...?

@NiTRoeSE
Copy link
Contributor Author

it doesnt have to ... For me generate-images looks like a relict from the past ...

I'm sorry if I have initiated a discussion of principles about the icon now.
I just asking to better understand why this is done in this way. ( I'm newby to this project ;) )
For me its also okay if the commit is reverted. I think its also possible to achieve the goal simply through customizing possibilities from Gitea. So i just want to say.. i completely understand if there is no time to investigate in such a small cosmetic "problem". ;)

If you guys want to investigate and need icons made with photoshop , then i will gladly offer my help.

@6543
Copy link
Member

6543 commented Jan 29, 2020

@NiTRoeSE I think just creat a new pull and add your the icon you mad with your photo-edit-software ;)

and hopefully this time we dont need to revert 😓

@6543
Copy link
Member

6543 commented Jan 29, 2020

@silverwind to:

If IOS can not deal with transparency, it's IOS ...

if i understand correctly rel="apple-touch-icon" is only take in account by apple?
so if we add a icon for apple devices it wont afect android/wind/co ...
so why not generate a icon for ios manualy :) as @NiTRoeSE proposed to create one

@silverwind
Copy link
Member

Fix PR: #10065

@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.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants