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

Added favicon #178

Merged
merged 4 commits into from Nov 21, 2020
Merged

Added favicon #178

merged 4 commits into from Nov 21, 2020

Conversation

LalitNM
Copy link
Contributor

@LalitNM LalitNM commented Nov 12, 2020

@peterjc, @JoaoRodrigues and @MarkusPiotrowski
Have a look at this pull request and let me know if something else can be merged with this pull request(e.g. removing apple-touch-icon-144.png

This fixes #86.

@peterjc
Copy link
Member

peterjc commented Nov 12, 2020

Could you update apple-touch-icon-144.png too? It still uses the old icon.

@LalitNM
Copy link
Contributor Author

LalitNM commented Nov 13, 2020

I updated apple-touch-icon.png. Have a look.

@peterjc
Copy link
Member

peterjc commented Nov 13, 2020

Was it deliberate to rename apple-touch-icon-144.png to apple-touch-icon.png? I'd have to remind my self of the Apple conventions here, but I thought the name might have some meaning.

@LalitNM
Copy link
Contributor Author

LalitNM commented Nov 13, 2020

After your comment I read about this on developer.apple.com. According to them it is enough to keep the file as apple-touch-icon.png. We can even remove the <link> from head.html.

The problem is with the resolution of image, maximum resolution is 180x180, but at present we are adding a file with resolution144x144. Should I add the image with higher resolution?

@MarkusPiotrowski
Copy link
Contributor

As far as I have understood it is practice now to provide several favicon.png files with different resolutions:
https://stackoverflow.com/questions/2268204/favicon-dimensions
https://stackoverflow.com/questions/4014823/does-a-favicon-have-to-be-32x32-or-16x16
https://www.emergeinteractive.com/insights/detail/the-essentials-of-favicons/

So you can decide to put something like 7-... different sized favicons to support an optimal look on each plattform/browser or to put something like 3 files, one being a 32x32 png, one being an high-resolution apple-touch icon and possibly a high-resolution png.

@LalitNM
Copy link
Contributor Author

LalitNM commented Nov 14, 2020

@MarkusPiotrowski idea of adding multiple images looks good. The problem I pointed out in this comment can be fixed by applying an image with transparent background. I am thinking to add html markup and images generated by realfavicongenerator.net. This will automatically add images for different devices.

@LalitNM LalitNM requested a review from peterjc November 14, 2020 12:09
Copy link
Member

@peterjc peterjc left a comment

Choose a reason for hiding this comment

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

Seems fine, but I am not up to date with best practises here. Over to @MarkusPiotrowski or @JoaoRodrigues to second and/or merge.

@LalitNM
Copy link
Contributor Author

LalitNM commented Nov 16, 2020

@JoaoRodrigues and @MarkusPiotrowski would you like to review this?

@MarkusPiotrowski
Copy link
Contributor

I have merged the changes to my local branch and pushed it here:
http://markuspiotrowski.github.io/

As far as I can tell it looks good on Windows 10 in Firefox and Edge browser but not so nice as tile (because the tile background is also blue). Actually, this was the first time that I added a webpage as "app" on Windows, so I think this is a very rare scenario and we can live with that.
It also looks nice on an Android tablet (as icon and in Chrome browser).
If anyone wants to check this on Apple devices, please do so, I'm going to merge this tomorrow.

@peterjc
Copy link
Member

peterjc commented Nov 21, 2020

Tiny tab/bookmark icon looks good on macOS Chrome, Firefox and Safari.

Home screen icon looks good on iPhone.

@MarkusPiotrowski MarkusPiotrowski merged commit 769d697 into biopython:master Nov 21, 2020
@MarkusPiotrowski
Copy link
Contributor

@LalitNM Good work, thank you!

@LalitNM LalitNM deleted the favicon branch November 25, 2020 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating a Biopython favicon
3 participants