-
Notifications
You must be signed in to change notification settings - Fork 18
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
Open graph meta tags #913
Open graph meta tags #913
Conversation
@codemonkey800 Is this the PR that needs to be merged before #895 can be worked on? If so, I will note it in that ticket. |
haha yeah it is, no worries I already linked it in the ticket |
title={collection?.title} | ||
url={ | ||
collection | ||
? `https://napari-hub.org/collections/${collection.title}` |
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.
Could this url be relative so we can point to the collection page in the same environment?
Also, does the title referenced here point to the name of collection, like "Segmentation Plugins by the Alfa Cohort" or the id
like "alfa-cohort"? Because we link to the collections by their id, like https://www.napari-hub.org/collections/alfa-cohort, but we might want the metadata title to be the descriptive name.
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.
ahh you're right I forgot about that, although since this is for SEO, it mostly only matters for the prod environment lol
Since the metadata is rendered on the server, the only way to use the correct URL is by checking for the ENV
variable. I pushed a commit to check that
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.
the id like "alfa-cohort"
also good catch, thank you!! it's supposed to be the symbol. I updated this in my recent commit too 😆
frontend/src/utils/url.ts
Outdated
base = 'https://staging.napari-hub.org'; | ||
} | ||
|
||
return createUrl(path, base).href; |
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.
Would the DEV env having the base be localhost
work?
Also, do the urls have to be absolute or could they be relative? If they could be relative, it might reduce the amount of processing we'd have to do.
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.
Would the DEV env having the base be localhost work?
originally we were skipping processing DEV
URLs for now since it can be dynamic, but I just remembered we have an env FRONTEND_URL
, so I pushed a commit to use this value
Also, do the urls have to be absolute or could they be relative?
hmm absolute is recommended for SEO:
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.
That clears things up. Thanks, I didn't realize this was for constructing the sitemap. I thought this was for the page metadata. 😅
description?: string; | ||
keywords?: string[]; | ||
} | ||
import { PageMetadataProps, usePageMetadata } from './usePageMetadata'; | ||
|
||
/** | ||
* Renders meta tags for the current page. If keywords are provided, they'll be |
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.
Since new code has been added, just wondering if the comment section here is still reflective of the changes?
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.
yeah I think it's fine to keep the description how it is now, it's still focused on rendering meta tags in this component
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.
Description
#900
Adds open graph meta tags to every page
Demos