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

Use realfavicongenerator API to build high-res PNG favicons #883

Merged
merged 24 commits into from Nov 13, 2018

Conversation

@Bisaloo
Copy link
Contributor

@Bisaloo Bisaloo commented Nov 11, 2018

fix #845, fix #869

This is still WIP but I'm putting it here if you want to have a first look and tell me if I'm going in the wrong direction. I still need to:

  • Add tests
  • Test with other input formats such as svg
  • Verify that docs are up to date everywhere
  • Add documentation for build_favicon()
  • ??

Among others, I would like you to review:

  • verbosity: more / less?
  • paths: path of the template, path of the resulting favicons
  • API key. At the moment, I'm using the demo API key but we probably need to register our own. One for all pkgdown users, hardcoded in the code? Asking users to register one themselves would introduce more complexity.
@Bisaloo Bisaloo changed the title Use realfavicongenerator API to build high-res PNG favicons [WIP] Use realfavicongenerator API to build high-res PNG favicons Nov 11, 2018
Copy link
Member

@hadley hadley left a comment

This is great and much simpler than I had anticipated! I have given you a bunch of comments to help make the code more robust (these are tips I've learned through getting them all wrong myself!)

I'm also wondering if it would be better to pull this out into a separate build_favicons() that the user would need to call explicitly? Otherwise we're potentially hitting this API every time we build the site. (We could store the favicons in pkgdown/favicons maybe?)

NEWS.md Outdated Show resolved Hide resolved
R/build-logo.R Outdated Show resolved Hide resolved
R/build-logo.R Outdated Show resolved Hide resolved
R/build-logo.R Outdated Show resolved Hide resolved
R/build-logo.R Outdated Show resolved Hide resolved
R/build-logo.R Show resolved Hide resolved
@hadley
Copy link
Member

@hadley hadley commented Nov 11, 2018

I think we should use a shared key that I register, providing an option for users to override if they want to provide their own.

@Bisaloo
Copy link
Contributor Author

@Bisaloo Bisaloo commented Nov 11, 2018

@hadley, thank you for your comments. I addressed some of them and I will probably have a look at the rest tomorrow.

I did the last changes in numerous small commits to make it easier to review. But once everything is ready to go, I can rebase & squash and force push.

I'm also wondering if it would be better to pull this out into a separate build_favicons() that the user would need to call explicitly? Otherwise we're potentially hitting this API every time we build the site. (We could store the favicons in pkgdown/favicons maybe?)

Yeah, I forgot, I was thinking we could maybe add a check at the beginning of this function or in build_site() and only go through with the API if the master logo.png hash has changed. What do you think? It would probably be simpler for users than calling build_favicons explicitly.

@jayhesselberth
Copy link
Collaborator

@jayhesselberth jayhesselberth commented Nov 11, 2018

Do we want a better default favicon while we're at this? Currently it's a (lo-res) blank black hex.

@hadley
Copy link
Member

@hadley hadley commented Nov 12, 2018

@Bisaloo I think it would be better to be explicit about it and force the user to call a function. (Also you don't need to squash on your end, as I can squash when merging)

@jayhesselberth I think we can ditch the default favicon now that we no longer need magick.

NEWS.md Outdated Show resolved Hide resolved
R/build-logo.R Outdated Show resolved Hide resolved
@Bisaloo
Copy link
Contributor Author

@Bisaloo Bisaloo commented Nov 12, 2018

So I just tested and this works perfectly for svg icons. find_logo() should thus probably be tweaked to detect logo.svg (and even prioritize it compared to logo.png). Should I do this here or in another PR?

R/build-logo.R Outdated Show resolved Hide resolved
Copy link
Member

@hadley hadley left a comment

Yes, should definitely prefer svg logos now. I think it's fine to include that here since it should be a fairly small change.

R/build-logo.R Show resolved Hide resolved
R/build-logo.R Show resolved Hide resolved
R/build-logo.R Outdated Show resolved Hide resolved
R/build-logo.R Outdated Show resolved Hide resolved
@hadley
Copy link
Member

@hadley hadley commented Nov 12, 2018

I'd suggest that you also run the function on pkgdown itself and check in the results.

@Bisaloo
Copy link
Contributor Author

@Bisaloo Bisaloo commented Nov 13, 2018

Sorry, I force pushed while fixing the conflicts. Only old commit that changed is related to NEWS.md. You stopped your review at 76c56a1. New commits start at 4984fd9.

R/build-logo.R Show resolved Hide resolved
@Bisaloo
Copy link
Contributor Author

@Bisaloo Bisaloo commented Nov 13, 2018

The API also creates a bunch of files that we don't use (favicon.ico, apple-touch-icon-60x60.png, apple-touch-icon-76x76.png, apple-touch-icon-120x120.png): should I remove them or add them to the head.html template?

Copy link
Member

@hadley hadley left a comment

I think we should include them to the template too.

We probably also need to make the template only include them if the favicons are actually present

R/build-logo.R Show resolved Hide resolved
R/build-logo.R Show resolved Hide resolved
R/init.R Outdated Show resolved Hide resolved
@Bisaloo
Copy link
Contributor Author

@Bisaloo Bisaloo commented Nov 13, 2018

We probably also need to make the template only include them if the favicons are actually present

I'm not sure how to do that 😕

@hadley
Copy link
Member

@hadley hadley commented Nov 13, 2018

In that case, let me do a final review of this work, and then I can merge it. Then I can figure out how to let the template know.

NEWS.md Outdated Show resolved Hide resolved
R/build-logo.R Outdated Show resolved Hide resolved
inst/templates/head.html Outdated Show resolved Hide resolved
inst/templates/head.html Outdated Show resolved Hide resolved
@hadley
Copy link
Member

@hadley hadley commented Nov 13, 2018

Are you happy for me to merge this now?

@Bisaloo Bisaloo changed the title [WIP] Use realfavicongenerator API to build high-res PNG favicons Use realfavicongenerator API to build high-res PNG favicons Nov 13, 2018
@Bisaloo
Copy link
Contributor Author

@Bisaloo Bisaloo commented Nov 13, 2018

Yeah, I just tested locally one last time and everything seems to work fine so I guess it's good to go.

@hadley
Copy link
Member

@hadley hadley commented Nov 13, 2018

Awesome, thank you so much for all your hard work on this!

@hadley hadley merged commit e876818 into r-lib:master Nov 13, 2018
1 of 2 checks passed
@Bisaloo
Copy link
Contributor Author

@Bisaloo Bisaloo commented Nov 14, 2018

Thank you for all your comments!

And don't forget to register your API key 😉

@hadley
Copy link
Member

@hadley hadley commented Nov 14, 2018

I signed up yesterday, but still haven't received the email 😢

@Bisaloo Bisaloo deleted the png-favicons branch Nov 14, 2018
krlmlr added a commit to ropensci/tic that referenced this issue Jan 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants