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

Store external favicons in localStorage #6

Closed
wants to merge 1 commit into from

Conversation

Gee19
Copy link
Contributor

@Gee19 Gee19 commented Oct 1, 2021

🎃 work in progress but open to feedback

will hopefully resolve #2

Copy link
Owner

@princefishthrower princefishthrower left a comment

Choose a reason for hiding this comment

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

This looks like a nice way to store them. I'd say you're nearly there. Now it's just a matter of setting the faviconRef.current.href to the base64 string you've stored. Keep in mind they should be merged back into the faviconURIs since it is possible for a user to mix both local and external favicon URIs. Can you try and add this? Also, what is draw the rest of the owl?

One small other thing. I think it's time we start creating util functions. Can you create a util folder under src and then a single file and export your convertToBase64 function from it? Also add the type annotation to externalFaviconURI, when you do so i.e. (externalFaviconURI: string) => ...

Let me know how it goes! This will be a great addition to the hook!

@Gee19
Copy link
Contributor Author

Gee19 commented Oct 2, 2021

@princefishthrower thanks for the comment, I'll tackle the stuff you mentioned soon. Also the owl comment is from a stupid meme.. I didn't sleep much the night I wrote this lol

@princefishthrower
Copy link
Owner

Screen Shot 2021-10-02 at 23 38 06

@Gee19 haha I remember now 😂 I've seen that one!

@Gee19
Copy link
Contributor Author

Gee19 commented Oct 2, 2021

Still needs to be tested but I implemented the requested changes.

@Gee19
Copy link
Contributor Author

Gee19 commented Oct 2, 2021

Would you be open to formatting stuff with prettier if I opened a separate PR?

@princefishthrower
Copy link
Owner

@Gee19 - did you at least smoke test it? Does it "work"? Feel free to add prettier in a separate PR as well!

@Gee19
Copy link
Contributor Author

Gee19 commented Oct 4, 2021

@Gee19 - did you at least smoke test it? Does it "work"? Feel free to add prettier in a separate PR as well!

My initial pass was working when I tested it in isolation. I haven't been able to test the new changes. I'll come back to this tomorrow

@Gee19
Copy link
Contributor Author

Gee19 commented Oct 5, 2021

I don't believe this approach will work because of CORS :(

@Gee19 Gee19 closed this Oct 5, 2021
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.

Add External Favicon Preloading Logic
2 participants