-
-
Notifications
You must be signed in to change notification settings - Fork 622
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
fix issue #1054 by converting png to ico and linking them #1068
Conversation
Basically, all this commit does is create and execute a script (`./customize.dist/favicon/convert.sh`) that uses ImageMagick to convert various PNGs to ICOs(the format of favicon). It also adds the favicons to the HTML document through `./www/common/notify.js`.
The last commit added `.ico` versions of the `png` favicons. This commit adds each favicon statically to each application's `index.html` file. By statically, I mean it is not inserted through Javascript and instead is present in the file already. This fixes favicons not displaying on Firefox.
I have added another commit to this branch. Now, Firefox will display icons for any bookmarked application. (I tested all of them) Do note: This commit erases the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Can you remove unnecessary console.debug() calls and add ';' where it is missing?
Yes, of course. Check now! |
@lemondevxyz, can you commit the script |
This PR breaks the "icon notification" (the red dot) when a document has changed |
@wginolas I added it. Do note is that it needs to be executed inside the As for the "icon notification", could you supply a screenshot of it? I tried going to https://cryptpad.fr - edited a document from different browsers and the favicon didn't change. |
Fixes #1054
Basically, all this commit does is create and execute a script (
./customize.dist/favicon/convert.sh
) that uses ImageMagick to convert various PNGs to ICOs(the format of favicon). It also adds the favicons to the HTML document through./www/common/notify.js
.This commit may not affect https://realfavicongenerator.net/ because I think it doesn't execute Javascript judging from how fast it loads the website. (Favicons are injected through
./www/common/notify.js
)However, it will work in Chrome. Firefox doesn't work with this patch because it doesn't use the same internal mechanism(My theory that it fetches the HTML and since favicon gets injected with Javascript unnecessarily, it won't load the favicon).
I haven't tested this with Apple Safari yet.
I do not mind modifying all the various
inner.html
in the different applications to ensure favicon works on Firefox but I am not sure if this path is something that CryptPad is interested in.