Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Fixes #4738 - Remove subtitle link favicon #4739

Merged

Conversation

punamdahiya
Copy link
Contributor

No description provided.

@punamdahiya punamdahiya requested a review from jaredhirsch August 3, 2018 23:13
@punamdahiya
Copy link
Contributor Author

@6a68 Attaching PR that removes the code that displays subtitle link favicon. Will file a follow up bug to restore it when upstream bug returning image instead of url is fixed

Copy link
Member

@jaredhirsch jaredhirsch left a comment

Choose a reason for hiding this comment

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

Sorry to disagree with my own advice here, but since this doesn't affect users on release or dev channels, it's probably better to just guard against showing the base64 favicons, then we won't have any extra work to do later. Thoughts?

@@ -400,12 +400,6 @@ class Body extends React.Component {
renderGetFirefox = renderExtensionNotification = false;
}

let favicon = null;
if (shot.favicon) {
Copy link
Member

Choose a reason for hiding this comment

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

@punamdahiya Hmm, on second thought, it might be better to keep showing the favicon for the vast majority of shots, which have a regular favicon URL, and just change this line to:

if (shot.favicon && shot.favicon.startsWith('https'))

Copy link
Member

@jaredhirsch jaredhirsch left a comment

Choose a reason for hiding this comment

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

After talking in IRC, it seems there are other problems with the proxy endpoint that break favicons in production anyway, so this couldn't hurt.

Merging 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants