-
-
Notifications
You must be signed in to change notification settings - Fork 504
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
refactor: Swap the OG image to one including the Preact name #1130
base: master
Are you sure you want to change the base?
Conversation
f98f525
to
bcb13c4
Compare
35169a1
to
3a9baab
Compare
<meta name="twitter:card" content="summary"> | ||
<link rel="alternate" type="application/rss+xml" href="%NETLIFY_DEPLOY_URL%/feed.xml"> | ||
<link rel="alternate" type="application/atom+xml" href="%NETLIFY_DEPLOY_URL%/feed.atom"> | ||
<meta property="og:type" content="website"> |
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.
Per the spec, we're meant to always have a og:type
meta tag.
website
seems the most fitting, but there seems to be a healthy mix of both website
and article
on similar frameworks' docs sites. Not 100% sure which is best, though article
seems like it carries additional meaning on some sites, so I went with website
const NETLIFY_DEPLOY_URL = process.env.CONTEXT === 'deploy-preview' | ||
? process.env.DEPLOY_PRIME_URL | ||
: 'https://preactjs.com'; | ||
|
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.
Makes it slightly easier to test some of this stuff out in deploy previews as you'd otherwise need to edit & then revert before merging.
This is the Netlify recommendation for doing this sort of thing -- CONTEXT
is going to be deploy-preview
or production
(can spot this by opening up the deploy logs, prod & preview) and DEPLOY_PRIME_URL
will be our preview URL (Docs)
<meta property="og:type" content="website"> | ||
<meta property="og:image" content="%NETLIFY_DEPLOY_URL%/og-image.png"> | ||
<meta name="twitter:card" content="summary_large_image"> | ||
<meta name="twitter:site" content="@preactjs"> |
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.
Supposedly this gives some sort of attribution and lets users jump right to the account (Docs)? Not quite sure how that's meant to work, or if it's currently broken, but the user data does come through (going by network pane in devtools), so figure it can't hurt.
We can also add <meta name="twitter:creator" content="@...">
tags for blog authors later, if we wanted (assuming that does something and/or gets fixed too).
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.
Amazing
48ce801
to
624c93b
Compare
There is admittedly a loss of "at a glance" information which I'm growing a bit conflicted on. Not all of our descriptions are great but I'm not sure if removing it outright is really adding anything (even if the full branding looks marginally better, IMO). Might let this sit for a bit. |
For comparison:
Links: This PR, Existing
Just an idea though, happy to revert if people prefer the original.