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

Highly customizable Favicon(s) feature #146

Merged
merged 5 commits into from
Jun 27, 2022

Conversation

geoffreygarrett
Copy link
Contributor

Hi, firstly I'd like to say thank you for such an incredible tool. I've made some interesting additions which may be very useful to more than just me. This PR includes my changes for highly customizable Favicons for your personal quartz website.

Without adding the favicon key to the data/config.yaml file for quartz, everything will generate as it did before.

For users who want to customize their Favicons themselves, perhaps as generated by Favicon generator websites, they can do so by defining a value to the favicon key.

The default in quartz:

<link rel="shortcut icon" href="icon.png" type="image/png">

Is equivalent to defining the favicon key in the data/config.yaml as:

favicon:
  - { rel: "shortcut icon", href: "icon.png", type: "image/png" }

or

favicon: |
  <link rel="shortcut icon" href="icon.png" type="image/png">

If you find this useful and the modifications aren't as desired, please provide feedback and I'll make adjustments for the future :)

@geoffreygarrett geoffreygarrett marked this pull request as ready for review June 27, 2022 20:24
Copy link
Owner

@jackyzha0 jackyzha0 left a comment

Choose a reason for hiding this comment

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

A few comments, but overall this looks great!

@@ -23,6 +23,40 @@ links: # Links to show in footer
link: https://github.com/jackyzha0
```

### HTML Favicons

If you would like to customize the favicons of your website, you can add them to the `data/config.yaml` file. The default without any set `favicon` key is:
Copy link
Owner

Choose a reason for hiding this comment

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

a short explainer on favicons (even just "the icon you see in the tab bar" or something) would be really nice


```yaml
favicon: |
<link rel="shortcut icon" href="icon.png" type="image/png">
Copy link
Owner

Choose a reason for hiding this comment

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

this should also have multiple favicons to match the text above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good point, fixed 🎯

{{ $favicon | safeHTML }}
{{ else }}
{{ range $favicon }}
<link rel="{{.rel}}" {{if .type}}type="{{.type}}"{{end}} {{if .sizes}}sizes="{{.sizes}}"{{end}} href="{{$.Site.BaseURL}}{{.href}}" />
Copy link
Owner

Choose a reason for hiding this comment

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

does the href here include the / ? this seems to do {{$.Site.BaseURL}}icon.png for the default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

{{$.Site.BaseURL}} is defined as https://quartz.jzhao.xyz/ by default in the config.toml, so this works in this case: https://quartz.jzhao.xyz/icon.png, but you're right, if the baseURL is defined without the trailing /, this would cause an issue. I added a / between the two template arguments, because afaik, ...///... shouldn't be an issue.

Initially assumed that `href` definitions should have `/...` as their
pattern, and `baseURL` would always end with `/`, however the omission
of `/` as the prefix of the former and suffix of the latter
simultaneously, would result in broken favicon paths. Final comment:
`..///...` is not breaking, which is worst case scenario with this fix.
Copy link
Owner

@jackyzha0 jackyzha0 left a comment

Choose a reason for hiding this comment

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

lgtm :)) thanks for the contribution!

@jackyzha0 jackyzha0 merged commit 7294196 into jackyzha0:hugo Jun 27, 2022
tomoyanonymous pushed a commit to tomoyanonymous/quartz-research-note that referenced this pull request Oct 20, 2023
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.

None yet

2 participants