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.

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

Added option to use legacy Reddit favicon #4699

Merged
merged 7 commits into from
Mar 9, 2018

Conversation

corylulu
Copy link
Contributor

@corylulu corylulu commented Mar 7, 2018

Could have made it not dependant on showUnreadCountInFavicon, but it would have made it uglier and I don't think it matters much.

@@ -216,6 +223,10 @@ const setupFaviconBadge = _.once(() => {
for (const f of favicons) {
if (f !== selectedFavicon) f.remove();
}
// browser should auto resolve to this if all get removed, but just in case, setting it explicitly
if (module.options.faviconUseLegacy.value) {
selectedFavicon.href = '//www.reddit.com/favicon.ico';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use redditstatic.com instead of reddit.com

"message": "Use Legacy Favicon"
},
"orangeredFaviconUseLegacyDesc": {
"message": "If you don't like the new orange Reddit favicon, revert to using the old Reddit icon instead."
Copy link
Collaborator

Choose a reason for hiding this comment

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

orangered*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it all fine now?

Copy link
Collaborator

@jewel-andraia jewel-andraia left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@erikdesjardins
Copy link
Collaborator

Build failure looks spurious

@erikdesjardins erikdesjardins merged commit 243ac1f into honestbleeps:master Mar 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants