search

twentyhq / favicon

100% free and open-source favicon provider
https://github.com/twentyhq/twenty
MIT License
238 stars 19 forks source link

Missing favicons for some href's #29

Closed gitstart-app[bot] closed 1 month ago

gitstart-app[bot] commented 2 months ago

This PR was created by GitStart to address the requirements from this ticket: FAVI-28. This ticket was imported from: FAVI-28

Description: \ \ Initial assumptions were that it doesn't retrieve svg icons and that there is an issue with absolute url paths. After investigation, it was discovered that the\ reasons are the icons not being retrieved are failing the ImageManipulation.isAlmostSquare(faviconFile) check in the uniqueFaviconFiles method if they are svg files and for some png files that are not almost square.

\ Added an additional check to see if the icons are svg or png icons and resize them to a size and shape that is almost a square shape

Refs:

28

Demo:

https://www.loom.com/share/7142bff244f44311a299e5e915c4cb0b?sid=5d3622f5-3d20-4e48-8b8e-3c15c3973e2c

Fixes #28

charlesBochet commented 1 month ago

Interesting, @gitstart-twenty could you check that your PR is still working with a most of these domains: https://gist.github.com/jgamblin/62fadd8aa321f7f6a482912a6a317ea3 (no need to check all, let's check ~30 that looks important to you).

If no issues with these domains, LGTM

gitstart-twenty commented 1 month ago

Hello @charlesBochet here the tests we did on the list of the domains

https://www.loom.com/share/b19e363a2a12456a8bd43bbe3aac5b38?sid=4f2d2227-c39a-4042-912f-4d1b60417b0b https://www.loom.com/share/e7c64814a7cf4a6f95bcc110232aaee4?sid=b8159b73-b857-43f4-937e-f5879a2da788

From the test done the first 30 domains are working as expected as shown in the loom video, but the test was continued for other domains and an issue was discovered in t.co due to unsupported file format, but this is not related to this PR (also happens on the main branch). We could only reproduce this locally, probaly because the production version has cache for this icon.

charlesBochet commented 1 month ago

Thank you!