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

Restore simple configuration of favicon.ico #1095

Merged
merged 1 commit into from Dec 27, 2022

Conversation

pdmosses
Copy link
Contributor

@pdmosses pdmosses commented Dec 27, 2022

Avoid the need to add a link to favicon.ico when editing _includes/head_custom.html, and avoid creating an invalid favicon link

  • Remove the content of _includes/head_custom.html
  • Add code to _includes/head.html to create a link to an existing favicon.ico
  • Add an explanation of the use of favicon_ico to docs/configuration.md
  • Remove the example of _includes/head_custom.html, and add an explanation of what the <head> element automatically includes, in docs/customization.md

See this comment by @MichelleBlanchette and my subsequent comments for some background.

To some extent, this PR reverts PR #1027.

To test

  1. Build and serve this PR branch locally.

  2. Move favicon.ico to assets/images/, then rebuild this website.

  3. Check that the favicon has disappeared, and that Jekyll does not report "ERROR '/favicon.ico' not found".

  4. Set favicon_ico: /assets/images/favicon.ico in _config.yml, then rebuild this website.

  5. Check that the favicon has reappeared.

  6. Move the favicon back to the root directory, and remove the setting of favicon_ico.

Avoid the need to add a link to favicon,ico when editing `_includes/head_custom.html`, and avoid creating an invalid favicon link

- Remove the content of `_includes/head_custom.html`
- Add code to `_includes/head.html` to create a link to an existing favicon,ico
- Add an explanation of favicon_ico to docs/configuration.md
- Remove the example of `includes/head_custom.html` and add an explanation of what the `<head>` element automatically includes
Copy link
Member

@mattxwang mattxwang left a comment

Choose a reason for hiding this comment

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

Very elegant, I love this! Thanks for taking the time to implement it. Using site.static_files didn't even come to mind when I originally wrote this feature.

@mattxwang mattxwang merged commit 7cabda2 into just-the-docs:main Dec 27, 2022
Copy link
Contributor

@MichelleBlanchette MichelleBlanchette left a comment

Choose a reason for hiding this comment

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

Yes! This makes more sense now. Thank you for understanding my concern and addressing this @pdmosses 🙌

@pdmosses
Copy link
Contributor Author

@mattxwang Thanks for looking at this issue again – and so quickly!

I guess I spent more time looking for a simpler way of testing for existence of a named file than actually coding it. Perhaps the best place to credit as prior art is Jekyll issue #7528.

@pdmosses
Copy link
Contributor Author

@MichelleBlanchette I should apologise for originally being too dismissive in my response to your original comment. I'd understood your concern, but I hadn't seen how we might address it better.

@Potherca's objection to the use of head_custom.html for the default favicon link was also a helpful nudge in the right direction.

@pdmosses pdmosses deleted the restore-simple-favicon branch December 27, 2022 22:06
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

3 participants