[docs-infra] Add og:url tag
#3415
Conversation
pondorasti
force-pushed
the
al/feat-og-url-tag
branch
from
3f39a76 to
c26fc95
Compare
pondorasti
requested review from
atomiks,
colmtuite and
michaldudak
as code owners
commit: |
Bundle size report
Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
LukasTy
added
the
scope: docs-infra
| ); | ||
| } | ||
|
|
||
| export const metadata: Metadata = { |
There was a problem hiding this comment.
So those settings would apply to both public and private (experiments) pages.
There was a problem hiding this comment.
It doesn't, but it already has the other og: attributes.
Do you see any problem with the colocation of the config?
There was a problem hiding this comment.
Why move this? @pondorasti
Just cleaning up the code a bit. imo, it's much easier to follow things around when metadata configuration is collocated in one file and there's no layout inheritance at play.
I would only keep the metadata from /(public)/layout only if it has a functional role.
LukasTy
left a comment
LukasTy
left a comment
There was a problem hiding this comment.
LGTM. 👍
However, are we good with the fact, that og:url will always point to the production link, even though we might be on a PR deployed docs version.
Would using a current domain be possible and make more sense?
WDYT @mui/infra?
It's likely possible, but I'm not so sure what some of the benefits would be. I guess it comes down to meta tag testing, i.e. if you want to test opengraph previews and make them point back to the netlify preview url. |
| }; | ||
|
|
||
| export const metadata: Metadata = { | ||
| metadataBase: new URL('https://base-ui.com'), |
There was a problem hiding this comment.
We do have process.env. SITE_DEPLOY_URL that'll have the correct value based on the environment. Don't need to hard code.
There was a problem hiding this comment.
I don't think SITE_DEPLOY_URL would suffice as I believe it contains https://master--base-ui.netlify.app/ on production deploys. I think the desired behavior would be
- production:
base-ui.com=URL - PRs:
deploy-preview-<pr-number>--base-ui.netlify.app=DEPLOY_PRIME_URL - legacy:
v1.base-ui.com= ? (currently not necessary yet for base, but it is for core and X)
I believe we should encode this in withDeploymentConfig() function in infra. We can base it off of the method used by llms.txt generation:
const NETLIFY_DEPLOYMENT_URL =
process.env.PULL_REQUEST === 'true' ? process.env.DEPLOY_PRIME_URL : process.env.URL;We should just add an optional variable in the mix for legacy deployments, e.g.
// whatever we know from Netlify
const NETLIFY_DEPLOYMENT_URL =
process.env.PULL_REQUEST === 'true' ? process.env.DEPLOY_PRIME_URL : process.env.URL;
// optional override from DEPLOYMENT_URL variable
const DEPLOYMENT_URL = process.env.DEPLOYMENT_URL || NETLIFY_DEPLOYMENT_URL;There was a problem hiding this comment.
Thanks, Jan, I think this makes the most sense. 👍
If we ever need subdomains, it should also cover this use case the best. 🤔
@pondorasti, could you update the solution to use the suggested URL resolution method?
6 participants
see #1615, closes mui/mui-public#561
Continuation of #3388:
og:urltags for all pagesmetadataBaseto the root layout