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.

Sign up for GitHub

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

Jump to bottom

fix: fix helpful link thumbnail issue #738

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

fix: fix helpful link thumbnail issue #738

wants to merge 1 commit into from
+5 −8

Conversation

CK-7vn
Copy link
Member

@CK-7vn CK-7vn commented Feb 13, 2025

Description of the change

Thumbnails on helpful link's weren't being rendered properly because at times the response status code would actually be 200, but there still would be no favicon. We fix this by checking whether or not the actual content-length of the response is 0 or less, and if it is, assign the ULL logo.

Screenshot(s)

Reference the few links copied from staging that weren't working
image

@CK-7vn CK-7vn force-pushed the helpful_favicons branch 2 times, most recently from 17a1e9a to c94e1b6 Compare February 13, 2025 17:53
@jtucholski
Copy link
Contributor

@PThorpe92 @CK-7vn Just a thought. Most of the favicons identified in the original issue don't follow the convention /favicon.ico but they can be determined by looking at the HTML for the page

<head>
  <link rel="icon" href="https://211maine.org/wp-content/uploads/2017/10/211_favicon.png" sizes="32x32">
</head>

Is it worth it to send a request out to the URL and look for the icon that the site provides?

Second question, do we hotlink to their image like we do today, knowing that it could change?

@PThorpe92
Copy link
Member

@jtucholski yeah we might as well grab their TLD index.html page, find the favicon and then store it (like we do in upload handler)

@corypride corypride self-requested a review February 14, 2025 18:40
@jtucholski jtucholski requested review from jtucholski and corypride and removed request for corypride February 14, 2025 19:15
jtucholski
jtucholski requested changes Feb 14, 2025
View reviewed changes
Copy link
Contributor

@jtucholski jtucholski left a comment

Choose a reason for hiding this comment

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

Based on our discussion in Slack we're going to see if we can scrape the <link> tag from the page and detect the path for the favicon.

@CK-7vn
Copy link
Member Author

CK-7vn commented Feb 15, 2025

I am going to argue that we use the very simple solution I just provided....no one shoot me,
google has this hidden away API I found that literally you just add the domain into the link, and it gets every icon, the screenshot below is literally all icons that aren't working on staging, I havent found a link yet that didnt return an icon with this thing.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtucholski jtucholski jtucholski requested changes

@corypride corypride Awaiting requested review from corypride

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Backend
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BUG: Favicons Not Displaying on Helpful Links Cards
3 participants
@CK-7vn @jtucholski @PThorpe92