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

add default favicon to templates #1116

Merged
merged 8 commits into from Apr 3, 2024
Merged

add default favicon to templates #1116

merged 8 commits into from Apr 3, 2024

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Mar 20, 2024

TODO:

  • design an asset (and format) to use as a default favicon for projects
  • apply same code to the other template
  • see if we want to revive favicon #633
  • see if we use autodetection of the favicon.png asset (in observablehq.config.ts or in normalizeconfig)?

closes #674

@Fil Fil requested review from mbostock and pettiross March 20, 2024 10:05
@mbostock
Copy link
Member

Safari doesn’t support SVG favicons; you have to use the nonstandard mask-icon. 😞

@Fil
Copy link
Contributor Author

Fil commented Apr 2, 2024

Capture d’écran 2024-04-02 à 20 22 08
Capture d’écran 2024-04-02 à 20 22 01

asset supports both dark and light modes (by @pettiross); minified down to 493 bytes

@Fil Fil marked this pull request as ready for review April 2, 2024 18:34
@Fil Fil enabled auto-merge (squash) April 2, 2024 18:35
@Fil Fil disabled auto-merge April 2, 2024 19:25
@Fil Fil enabled auto-merge (squash) April 2, 2024 19:25
@mbostock
Copy link
Member

mbostock commented Apr 2, 2024

Can we not use our official favicon? I like it better. I don’t like that hairline white halo in dark mode.

https://static.observablehq.com/favicon-512.0667824687f99c942a02e06e2db1a060911da0bf3606671676a255b1cf97b4fe.png

@Fil Fil disabled auto-merge April 3, 2024 02:58
@Fil
Copy link
Contributor Author

Fil commented Apr 3, 2024

OK to change the icon.

I thought of a different way to implement this that would make it easier to opt in and out: when we normalize the config, and if head is undefined, we could check for the existence of /favicon.png and if present, set the option to <link rel="icon" href="favicon.png" type="image/png">. Sounds nice?

@mbostock
Copy link
Member

mbostock commented Apr 3, 2024

Hmm, checking for favicon.png feels a little magical to me — it’s not hard to set the head option, is it? Especially if we now show you how to do it with the default template?

@Fil
Copy link
Contributor Author

Fil commented Apr 3, 2024

I was thinking it could be useful for a beginner who does not open the config file and removes favicon.png from docs/. But the current code is working in a good way in that situation: the generated page points to the non-existing /favicon.png, a 404 is shown in the network inspector, all the information is there and nothing is badly broken.

Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

When you’ve replaced the image with the gray version let me know and I’ll take another look.

I am also tempted to call it observable.png rather than favicon.png, and to set sizes="32x32", so as to encourage authors to use a differently-named file if they want to replace it, and thereby force discovery of the head option if you want to change the favicon. But not a requirement.

@Fil
Copy link
Contributor Author

Fil commented Apr 3, 2024

Yes I'm only hesitant to use a 15kB 512x512 image, when we could minify a 32x32 image for 394 bytes. (It's not on the hot path, so not a big deal.)

@mbostock
Copy link
Member

mbostock commented Apr 3, 2024

You can use gm convert and optipng to convert that 512x512 image to 32x32? I’m not opposed to having a more minified favicon. I just prefer the gray one to the black & white one.

@Fil
Copy link
Contributor Author

Fil commented Apr 3, 2024

Done. I also tested a build, just in case :)

templates/default/observablehq.config.js.tmpl Outdated Show resolved Hide resolved
Fil and others added 2 commits April 3, 2024 20:08
Co-authored-by: Mike Bostock <mbostock@gmail.com>
@Fil Fil enabled auto-merge (squash) April 3, 2024 18:09
@Fil Fil merged commit d4efad7 into main Apr 3, 2024
4 checks passed
@Fil Fil deleted the fil/favicon branch April 3, 2024 18:14
@mbostock mbostock mentioned this pull request Apr 3, 2024
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.

observablehq-create should include a favicon
2 participants