Skip to content

Conversation

@serjan-nasredin
Copy link
Contributor

@serjan-nasredinserjan-nasredin commented Apr 17, 2021

Fix#496

Not sure. Check this, please.

<title>{{page.title }}</title>
<metaname="viewport" content="width=device-width">
<linkrel="icon" type="image/x-icon" href="../images/logo.png"/>
<linkrel="icon" type="image/x-icon" href="images/logo.png"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that @sivaraam suggested either /images/logo.png or just adding a link to it or a copy of it named favicon.ico. But this is doing something else. Is there a reason why you think Kaartic's suggestions will not work, or what you are doing is better?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

If I'm not mistaken, this is how it works:

_layouts L default.html images L logo.png 

And what I did then - it worked correctly, but I don't know why: rev_news / rev_news.md do not work.
But here's what confused me:
#496 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

As far as I understand, paths in the layout are referenced relatively from the path where the layout is used. So, your relative reference worked properly for the index and about pages but not for Rev News pages.

That's why I suggested using an absolute path instead.

Copy link
Member

@sivaraamsivaraam left a comment

Choose a reason for hiding this comment

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

The favicon.ico file should be placed only at the ROOT of the repository. No need to add it to sub-folders. That way the favicon is automatically served to the browser upon request.

On some research though, it looks like using the link tag is the standardized way to serve website icons [ref]. See suggestion about how to do that below.

<title>{{page.title }}</title>
<metaname="viewport" content="width=device-width">
<linkrel="icon" type="image/x-icon" href="../images/logo.png"/>
<linkrel="icon" type="image/x-icon" href="images/icons/favicon.ico"/>
Copy link
Member

Choose a reason for hiding this comment

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

If we're going the favicon.icon route, you could just remove the whole link tag altogether. It isn't necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, we could fix this link tag to make it work with the logo by using an absolute link as follows:

Suggested change
<linkrel="icon" type="image/x-icon" href="images/icons/favicon.ico"/>
<linkrel="icon" type="image/x-icon" href="/images/logo.png"/>

@serjan-nasredin
Copy link
ContributorAuthor

serjan-nasredin commented Apr 30, 2021

Hello. Sorry for the lack of feedback.
@sivaraam, check it out.

Copy link
Member

@sivaraamsivaraam left a comment

Choose a reason for hiding this comment

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

Hello. Sorry for the lack of feedback.

No worries! 🙂

@sivaraam, check it out.

This looks good to me. Thanks for the fix @snxx-lppxx !

@sivaraamsivaraam merged commit 4a7b445 into git:masterMay 2, 2021
@serjan-nasredinserjan-nasredin deleted the patch-2 branch May 2, 2021 13:01
Sign up for freeto 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.

Favicon: missing

3 participants

@serjan-nasredin@chriscool@sivaraam