Fix favicon "not found" error on non-homepage pages #617

vidipsingh · 2024-12-29T09:45:52Z

This PR addresses the issue where favicons were not found on non-homepage pages, particularly on Firefox browser. The problem was traced to an incorrect favicon path that was not being handled properly across different pages.

Changes Made:

Updated the favicon link in _layouts/base.html to use a consistent, absolute path.
Modified js/main.js to dynamically update the favicon based on a random color scheme.
Implemented error handling in the JavaScript to revert to a default favicon if the specific one is not found.

Issue Link:

Closes #507

How to Test:

Load the homepage and any non-homepage page (e.g., /about-us or /press) on Firefox.
Check if the favicon is displayed correctly across all pages.
Ensure that a fallback favicon is shown if the dynamic one is missing.

Additional Notes:

This issue was specifically observed in the Firefox browser.
The fix ensures consistent favicon display on all pages, improving the site's visual consistency and user experience.

OS and Browser Information:

Operating System: Linux
Browser: Firefox

quozl · 2024-12-29T20:23:23Z

You have a commit to add author name display to blog, yet this pull request is for the favicon problem. Please remove the commit 9c1b988.

Also, you have placed in the pull request opening comment text that should be in the commit message of 53cb483. Please do read our guide to making commits carefully.

- Updated `_layouts/base.html` to use a consistent favicon path. - Modified `js/main.js` to handle dynamic favicon changes with error handling. - Added fallback to the default favicon if a specific favicon is not found.

vidipsingh · 2024-12-30T05:45:39Z

@quozl, I have removed the commit 9c1b988 and will make sure to follow the guide for future contributions.
Thank you for the feedback!

quozl

I'm not convinced this is the right approach. Details are in the feedback below.

rewrite commit messages as suggested earlier.

quozl · 2024-12-30T20:47:39Z

_layouts/base.html

@@ -4,7 +4,7 @@
        <meta charset="utf-8">
        <meta name="viewport" content="width=device-width, initial-scale=1">
        <meta http-equiv="X-UA-Compatible" content="IE=edge">
- 		<link id="defaultIcon1" rel="shortcut icon" href="{{ site.baseurl }}assets/favicon_06.png" />
+        <link id="defaultIcon1" rel="shortcut icon" href="/assets/favicon.png" />


Why is site.baseurl removed?

Why is there no file assets/favicon.png yet you link to it here?

I will revert the changes in _layouts/base.html to the previous version.
Since there is no /favicon.png, should I refer to /assets/favicon_06.png here instead as before?

Don't mind either way, just that it should refer to something that exists.

Sure, then I will change it to {{ site.baseurl }}/assets/favicon_06.png.

quozl · 2024-12-30T20:53:15Z

js/main.js

+        };
+        img.onerror = function() {
+            // Fallback to default favicon
+            defaultIcon.href = '/assets/favicon.png';


There is no file assets/favicon.png

Since there is no /assets/favicon.png, should I refer to /assets/favicon_06.png here instead?

quozl · 2024-12-30T20:54:17Z

js/main.js

+        // Use absolute path for favicon
+        var faviconPath = '/assets/favicon_' + logoID + '.png';
+
+        // Test if favicon exists before setting


I don't see why we need to test if it exists. We know exactly how many files we have, what their names are, and we know the code above this section in js/main.js.

Sure, I'll remove the favicon test since we already know the number and names of the images.

quozl · 2024-12-30T20:57:44Z

js/main.js

@@ -16,7 +16,24 @@ $(document).ready(function () {
    if (logoID < 10) {
        logoID = "0" + logoID;
    }
-    document.querySelector('#defaultIcon1').href = 'https://www.sugarlabs.org/assets/favicon_' + logoID + '.png';


Code you are removing has nothing to do with the issue; the issue and my tests of master branch just now show the error to be a reference to press/assets/favicon_06.png and this line you are removing is an absolute link without press.

I have simplified the favicon logic by making the path relative to /press/, removing the full domain URL, and eliminating unnecessary existence checks, as shown in the following code:

var defaultIcon = document.querySelector('#defaultIcon1'); if (defaultIcon) { var logoID = colorIndex + 1; if (logoID < 10) { logoID = "0" + logoID; } defaultIcon.href = '/press/assets/favicon_' + logoID + '.png'; }

Also, I have removed the testing and existence checks. Is this approach fine?

You've lost me here. Why would the path be made absolute from /press/?

Sorry, I think relative path will be better for this like:

var defaultIcon = document.querySelector('#defaultIcon1'); if (defaultIcon) { var logoID = colorIndex + 1; if (logoID < 10) { logoID = "0" + logoID; } defaultIcon.href = 'assets/favicon_' + logoID + '.png'; }

Now it's pointing to the assets/ directory relative to the current location, which should be more flexible.

vidipsingh · 2024-12-31T11:04:19Z

I'm not convinced this is the right approach. Details are in the feedback below.

rewrite commit messages as suggested earlier.

Hi @quozl,

Thanks for the feedback. I’ll work on the changes as suggested.
Could you please point out what specifically is wrong with my current commit message? It would help me improve from next time.

quozl · 2024-12-31T21:33:33Z

Sure. When I went to your commit message and read it, assuming the persona of a future developer trying to figure out why git bisect identified the commit as the cause of a problem being investigated, I could not find the information we expect;

explain the problem that is solved; you've put this in the pull request opening comment instead of the commit message,
when the problem was introduced by a previous commit, mention the hash; this shows your full understanding and helps with review, letting us see how the problem began and evaluate if your solution is the right one,
when the problem is in an issue or ticket, as it does, end with "Fixes #507", so that GitHub will automatically close the issue.

It is alarming when a pull request opening comment has such a huge amount of detail that is left out of the commit message.

Remember that the git commit message is kept in perpetuity, and pull requests are deleted eventually. The git repository clone on a developer's system is where everything necessary to development must be present. GitHub is only a public hosting point for the repository.

vidipsingh · 2025-01-02T17:47:52Z

Sure. When I went to your commit message and read it, assuming the persona of a future developer trying to figure out why git bisect identified the commit as the cause of a problem being investigated, I could not find the information we expect;

explain the problem that is solved; you've put this in the pull request opening comment instead of the commit message,

when the problem was introduced by a previous commit, mention the hash; this shows your full understanding and helps with review, letting us see how the problem began and evaluate if your solution is the right one,

when the problem is in an issue or ticket, as it does, end with "Fixes #507", so that GitHub will automatically close the issue.

It is alarming when a pull request opening comment has such a huge amount of detail that is left out of the commit message.

Remember that the git commit message is kept in perpetuity, and pull requests are deleted eventually. The git repository clone on a developer's system is where everything necessary to development must be present. GitHub is only a public hosting point for the repository.

Thanks for the feedback, @quozl! I’ll make sure to incorporate these practices in future commits and ensure all relevant details are included directly in the commit message.

quozl · 2025-01-02T19:38:27Z

Thanks for the feedback, @quozl! I’ll make sure to incorporate these practices in future commits and ensure all relevant details are included directly in the commit message.

So you're saying you won't fix it for this pull request? Why not? Our purpose for review includes managing the history.

You have just one commit. All you have to do is git commit --amend followed by git push --force. If you had more than one commit, you would git rebase -i, select reword, then git push --force.

See our guide for reviewers to understand our review practices, and please learn to review your own work and others. We need more reviewers.

vidipsingh · 2025-01-03T06:37:33Z

Thanks for the feedback, @quozl! I’ll make sure to incorporate these practices in future commits and ensure all relevant details are included directly in the commit message.

So you're saying you won't fix it for this pull request? Why not? Our purpose for review includes managing the history.

You have just one commit. All you have to do is git commit --amend followed by git push --force. If you had more than one commit, you would git rebase -i, select reword, then git push --force.

See our guide for reviewers to understand our review practices, and please learn to review your own work and others. We need more reviewers.

Sorry about that, @quozl! I’ll make the necessary changes to the recent commit and ensure to follow these practices for all future commits as well.

quozl · 2025-01-03T20:14:35Z

@pikurasa ready for your review; this pull request has finished technical review and is on the way toward commit message goodness, just need to know you are happy with the solution to the issue.

pikurasa · 2025-01-04T21:29:18Z

js/main.js

+        if (logoID < 10) {
+            logoID = "0" + logoID;
+        }
+        defaultIcon.href = 'assets/favicon_' + logoID + '.png';


Is this still taking a relative path?

I ask because I still see things like this in the logs:

Agreed, is use of relative path. Oops.

Is this still taking a relative path?

I ask because I still see things like this in the logs:

Yes, it is now taking relative paths as intended. Previously, the code was referencing the favicon like this:

document.querySelector('#defaultIcon1').href = 'https://www.sugarlabs.org/assets/favicon_' + logoID + '.png';

However, I have updated it to use relative paths, as shown here:

defaultIcon.href = 'assets/favicon_' + logoID + '.png';

After this change, I am no longer seeing any errors in my logs, whether on Firefox or other browsers.

Well, relative or absolute, doesn't matter, as long as the directory that contains the icons is accessed on both the root and lower pages. It works for the root page, but as @pikurasa said it doesn't work on lower pages, such as about-us/. That's what you need to fix now.

(a relative access from about-us/ would be to ../assets/)

Well, relative or absolute, doesn't matter, as long as the directory that contains the icons is accessed on both the root and lower pages. It works for the root page, but as @pikurasa said it doesn't work on lower pages, such as about-us/. That's what you need to fix now.

In the _layouts/base.html, the previous line was:

<link id="defaultIcon1" rel="shortcut icon" href="{{ site.baseurl }}assets/favicon_06.png" />

I changed it to:

<link id="defaultIcon1" rel="shortcut icon" href="{{ site.baseurl }}/assets/favicon_06.png" />

The issue was that there was no forward slash between {{ site.baseurl }} and assets, which caused incorrect path resolution when navigating between pages like /about-us/. Now, the problem of the favicon "not found" error on non-homepage pages is not there in Firefox.

quozl · 2025-01-04T22:25:26Z

Noted edited commit message on 1939ac9

Review comments;

the first line is too long, see how GitHub renders it by breaking in the middle of a word, which is them telling you to fix it,
remove the # before the commit hash so it is properly rendered by GitHub, like this 58177ea,
put the "fixes" and issue number at the very end.

- Fixed "favicon not found" error on non-homepage pages, particularly in Firefox. - Problem traced to incorrect favicon path introduced in commit 58177ea. - Updated path from absolute to relative to resolve the issue. - Ensures favicons load correctly across all pages. Fixes sugarlabs#507

vidipsingh mentioned this pull request Dec 29, 2024

Favicon gets "not found" error on non-homepage pages #507

Open

Fix favicon "not found" error on non-homepage pages

58177ea

- Updated `_layouts/base.html` to use a consistent favicon path. - Modified `js/main.js` to handle dynamic favicon changes with error handling. - Added fallback to the default favicon if a specific favicon is not found.

vidipsingh force-pushed the fix/favicon-not-found-507 branch from 53cb483 to 58177ea Compare December 30, 2024 05:11

quozl requested changes Dec 30, 2024

View reviewed changes

quozl requested a review from pikurasa January 3, 2025 20:12

pikurasa reviewed Jan 4, 2025

View reviewed changes

vidipsingh force-pushed the fix/favicon-not-found-507 branch from 1939ac9 to 15f9065 Compare January 5, 2025 11:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix favicon "not found" error on non-homepage pages #617

Fix favicon "not found" error on non-homepage pages #617

vidipsingh commented Dec 29, 2024

quozl commented Dec 29, 2024

vidipsingh commented Dec 30, 2024

quozl left a comment

quozl Dec 30, 2024

vidipsingh Dec 31, 2024

quozl Dec 31, 2024

vidipsingh Jan 2, 2025

quozl Dec 30, 2024

vidipsingh Dec 31, 2024

quozl Dec 31, 2024

vidipsingh Jan 2, 2025

quozl Dec 30, 2024

vidipsingh Dec 31, 2024

quozl Dec 30, 2024

vidipsingh Dec 31, 2024

quozl Dec 31, 2024

vidipsingh Jan 2, 2025

vidipsingh commented Dec 31, 2024

quozl commented Dec 31, 2024 •

edited

Loading

vidipsingh commented Jan 2, 2025

quozl commented Jan 2, 2025

vidipsingh commented Jan 3, 2025

quozl commented Jan 3, 2025

pikurasa Jan 4, 2025

quozl Jan 4, 2025

vidipsingh Jan 5, 2025

quozl Jan 5, 2025

quozl Jan 5, 2025

vidipsingh Jan 6, 2025

quozl commented Jan 4, 2025

Fix favicon "not found" error on non-homepage pages #617

Are you sure you want to change the base?

Fix favicon "not found" error on non-homepage pages #617

Conversation

vidipsingh commented Dec 29, 2024

Changes Made:

Issue Link:

How to Test:

Additional Notes:

OS and Browser Information:

quozl commented Dec 29, 2024

vidipsingh commented Dec 30, 2024

quozl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vidipsingh commented Dec 31, 2024

quozl commented Dec 31, 2024 • edited Loading

vidipsingh commented Jan 2, 2025

quozl commented Jan 2, 2025

vidipsingh commented Jan 3, 2025

quozl commented Jan 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quozl commented Jan 4, 2025

quozl commented Dec 31, 2024 •

edited

Loading