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

feat(partials): allow setting of favicon path #16

Merged
merged 1 commit into from
Aug 21, 2024

Conversation

mesmur
Copy link
Contributor

@mesmur mesmur commented Aug 20, 2024

I was trying to set up the favicon for my hugo website but was having no luck with it.

The wiki simply told me to copy my favicon files into the static directory but for some reason that just led to no favicon at all.

When inspecting the head.html partial I saw that there was no <link> tag setting the favicon
(I'm not too experienced in frontend so I'm not sure how the default favicon was being set at all?)

This PR should make the favicon setup explicit, it also allows setting the path based on preference using a param. The fallback is set to the favicon.ico value which should be current state (except made explicit) so as to not make this a breaking change

@tomfran
Copy link
Owner

tomfran commented Aug 20, 2024

Thank you for the PR :)

Perhaps it's browser-dependent, what are you using not to have them displayed?
Anyway, better specify them as you did.

Can you please include also extra icons for IOS and Android? As in the static folder we also have the following:

  • apple-touch-icon.png
  • android-chrome-192x192.png
  • favicon-32x32.png
  • favicon-16x16.png

So we should include them all:

<link rel="icon" type="image/ico" href="/favicon.ico">
<link rel="icon" type="image/png" sizes="16x16" href="/favicon-16x16.png">
<link rel="icon" type="image/png" sizes="32x32" href="/favicon-32x32.png">
<link rel="icon" type="image/png" sizes="192x192" href="/android-chrome-192x192.png">
<link rel="apple-touch-icon" sizes="180x180" href="/apple-touch-icon.png">

Having a single param for each of those would be inappropriate, maybe you can specify a base folder path and hardcode the filenames.
This way a user could nest the icons in a static subfolder and specify the subpath in the config.

Tell me if this makes sense.

chore(partials): add explicit links for all favicon types
@mesmur mesmur force-pushed the feat/set-favicon-path branch from 2574bbb to 950382e Compare August 21, 2024 00:53
@mesmur
Copy link
Contributor Author

mesmur commented Aug 21, 2024

Perhaps it's browser-dependent, what are you using not to have them displayed?

Ah I tried on both arc (chromium) and firefox, but wasn't able to get things working even after resetting my cache.

Tell me if this makes sense.

Makes perfect sense!

I introduce a param faviconPath that the user can set as the base folder path.
The filenames were hardcoded as you suggested, I agree with that decision.

An example would be:

faviconPath = 'img' which would target the /img folder in the built public folder.

Leaving it empty would default to /, which is the current behavior and should be backwards compatible.


Also my auto-formatter made some minor changes in other sections to keep the style consistent. Let me know if that's not ok and I will revert those changes

@tomfran
Copy link
Owner

tomfran commented Aug 21, 2024

Looks good, thank you!

@tomfran tomfran merged commit 561fc71 into tomfran:main Aug 21, 2024
@achanda
Copy link

achanda commented Oct 28, 2024

Sorry if this is obvious, but I could not figure out how to set faviconPath. I tried setting it to ./static/favicon.ico but that did not work. I checked that my static dir does have favicon.ico, what am I missing?

tomfran added a commit that referenced this pull request Oct 30, 2024
feat(partials): allow setting of favicon path
tomfran added a commit that referenced this pull request Oct 30, 2024
feat(partials): allow setting of favicon path
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.

3 participants