enhance(mdim): set canonical url, include all views in sitemap, and include mdim title in page title#6607
enhance(mdim): set canonical url, include all views in sitemap, and include mdim title in page title#6607marcelgerber wants to merge 5 commits into
Conversation
|
Quick links (staging server):
Login:
Archive:SVG tester:Number of differences (graphers): 0 ✅ Edited: 2026-06-11 14:06:51 UTC |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3c0d8da2e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const value = searchParams.get(dim) ?? defaultChoice | ||
| newSearchParams.set(dim, value) |
There was a problem hiding this comment.
When a request includes a non-default or partial mdim query, this copies each incoming dimension value directly into the canonical URL and falls back to the global default for omitted dimensions. For sparse mdims, a URL like ?country=b can require a different default for a later dimension (or an incoming value can be invalid), while the client normalizes through filterToAvailableChoices; the canonical can therefore point at a view that is not actually rendered/available, creating duplicate or bogus canonical URLs for crawlers. Consider deriving the canonical dimensions from the same availability filtering used by the page, or only emitting combinations that exist in config.views.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
that's correct and unfortunate, but the only solution would be to include a comprehensive list of all available views and then filter them down.
I don't think we want to do that?
rakyi
left a comment
rakyi
left a comment
There was a problem hiding this comment.
I thought we wanted to treat a mutli-dim as the "page" and query params just as a way to interact with it and link to a specific view, similar to how search and latest work. Why do you think we need to change that? Is Ed aware of this proposed change?
Should we do the same for explorers?
ikesau
left a comment
ikesau
left a comment
There was a problem hiding this comment.
This makes sense to me! Tested it locally on wrangler and it all works as expected.
I say it's worth a try, but I'm open to arguments otherwise.
| loc: urljoin(BAKED_GRAPHER_URL, multiDim.slug), | ||
| lastmod: dayjs(multiDim.updatedAt).format("YYYY-MM-DD"), | ||
| })) | ||
| multiDims.flatMap((multiDim) => { |
There was a problem hiding this comment.
I just checked and it seems the max sitemap size is 50k
We're currently at ~6500
We currently have ~650 multidim views
We have ~6500 explorer views
So we should be okay with this approach without having to filter down the URLs by "high distinctiveness/value only"
| baseUrl={baseUrl} | ||
| staticAssetMap={assetMaps?.static} | ||
| archiveContext={archiveContext} | ||
| attrs={{ "data-owid-mdim-dimensions": mdimDimensions }} |
There was a problem hiding this comment.
Could we maybe call this -inital-view-dimensions to be completely clear how it works?
There was a problem hiding this comment.
Also, it would be better if we didn't nest it into an object. I know that we should not render this part on the client (for now), but always creating a new object with the same properties causes unnecessary re-renders and is a React antipattern.
| baseUrl={baseUrl} | ||
| staticAssetMap={assetMaps?.static} | ||
| archiveContext={archiveContext} | ||
| attrs={{ "data-owid-mdim-dimensions": mdimDimensions }} |
There was a problem hiding this comment.
Also, it would be better if we didn't nest it into an object. I know that we should not render this part on the client (for now), but always creating a new object with the same properties causes unnecessary re-renders and is a React antipattern.
| } | ||
| staticAssetMap?: AssetMap | ||
| archiveContext?: ArchiveContext | ||
| attrs?: Record<string, string | undefined> |
There was a problem hiding this comment.
A more idiomatic solution would be to extend the React.HTMLAttributes<HTMLHeadElement> type, and then we should be able to pass data- attribute as props to the <Head/> component.
There was a problem hiding this comment.
A more complete example:
interface HeadProps extends React.HTMLAttributes<HTMLHeadElement> {
title?: string
}
export function Head({ title, children, ...rest }: HeadProps) {
return (
<head {...rest}>
{title && <title>{title}</title>}
<meta charSet="utf-8" />
{children}
</head>
)
}4 participants
We noticed that some Mdim views, like
Number of newborns who have had a hepatitis B vaccine dose within the first 24 hours of delivery, are not findable on Google at all currently.This PR makes three changes to Mdims, essentially:
<link rel="canonical">in CF Functions, to ensure that the Mdim dimension choices are included in the canonical URL (so a?metric=totalwould be part of the canonical URL, but a?tab=mapwouldn't)Share of children vaccinated, by vaccine, World | Childhood vaccination coverage - by vaccine | Our World in DataThe sitemap change could have some negative SEO repercussions potentially, if Google finds that they don't like us indexing so many different views. But it's hard to know.
Antigravity says this:
Demo links: