Swap updated favicons #502
There are no files selected for viewing
|
There was a problem hiding this comment. Note: this image includes 20px of padding between the logo and the edge of the container per Evil Martian recommendations for how Apple displays touch icons. But, if we determine this isn't desirable, easy to change. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "name": "The Library Innovation Lab at Harvard University", | ||
| "icons": [ | ||
| { "src": "/assets/images/favicon-192.png", "type": "image/png", "sizes": "192x192" }, | ||
| { "src": "/assets/images/favicon-512.png", "type": "image/png", "sizes": "512x512" } | ||
| ] | ||
| } | ||
|
There was a problem hiding this comment. Building a PWA manifest is a big topic, so this isn't intended to be comprehensive for all what we would need to support an offline mode (and specifically to branding, for example, would we want to specify a maskable icon?). This is the minimum that Evil Martians recommends, so it felt like an easy add that we could adopt and update with a more comprehensive information in the future. Edit: After running a Lighthouse test to validate these icons, I decided we should support maskable icons. More details in the PR description above. There was a problem hiding this comment. Same, I'd recommend adding to the top of the file, Jekyll will make the substitution during build :-) There was a problem hiding this comment. Thanks for spotting this. As a quick heads-up, I'm still working on polishing this — it's not yet ready for review There was a problem hiding this comment. Also appreciate the heads-up about Jekyll's substitution process! |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though we aren't serving at a subpath, for style I'd recommend including
{{ site.baseurl }}here, that is,<link rel="manifest" href="{{ site.baseurl }}/manifest.webmanifest">. Right now, that's just set to"", so when Jekyll builds, the full URL becomes "/manifest.webmanifest", just like you have it. But, if we were to have Github pages serve at a subpath like it does by default (e.g. https://harvard-lil.github.io/website-static), we'd setbaseurlto/website-staticand when Jekyll builds, this URL would become "/website-static/manifest.webmanifest", and everything would continue to work.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for spotting this — this was a small oversight I planned to fix before marking this PR as ready for review