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 favicon.ico #756

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add favicon.ico #756

wants to merge 1 commit into from

Conversation

Konano
Copy link

@Konano Konano commented Sep 5, 2024

In the original intel, there is favicon.ico in the head section.

image

But after the IITC plugin, this line disappeared.

@mxxcon
Copy link

mxxcon commented Sep 5, 2024

But there's no actual icon file, unless I missed it...?

@Konano
Copy link
Author

Konano commented Sep 5, 2024

But there's no actual icon file, unless I missed it...?

I didn't add the ico file, I just added back the "shortcut icon" that was overwritten by IITC.

@modos189
Copy link
Contributor

In what cases is it required? When the icon is not displayed?

@xscreach
Copy link
Contributor

In what cases is it required? When the icon is not displayed?

This is just how to tell browser where to get icon for the site from. Browsers then use the icon for bookmarks and stuff like that.
The default url is iirc /favicon.ico...

This line is also present in the original intel html code.

But imho browser reads this info from the original html before it's rewritten by our stuff, we are not changing the value... so it might be kinda redundant, but maybe better save than sorry 🤷‍♂️

@Konano
Copy link
Author

Konano commented Sep 18, 2024

But imho browser reads this info from the original html before it's rewritten by our stuff, we are not changing the value... so it might be kinda redundant

Actually, browsers don't do that, from what I've seen... So if there is no icon url in the rewritten content, the browser will not load the icon.

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.

4 participants