-
Notifications
You must be signed in to change notification settings - Fork 37
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
fix(astro): better default meta tags #342
Conversation
Run & review this pull request in StackBlitz Codeflow. |
I'll take a look at the code next week, but another idea came into my mind: Should we create new So something like: import tutorialkit from '@tutorialkit/astro';
import { defineConfig } from 'astro/config';
export default defineConfig({
integrations: [
tutorialkit({
components: {
MetaData: './src/components/MetaTags.astro',
},
}),
],
}); And then in Improving default meta tags is important too. |
Definitely, I've thought about a Metadata component but then the question is how to pass the settings from a lesson to 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.
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.
I like this idea a lot! Thanks for the PR! 😃
I've left a couple of things to address. @AriPerkkio Or do you want to merge this as is and address the feedback in a follow-up PR?
@@ -17,7 +17,7 @@ type Props = InferGetStaticPropsType<typeof getStaticPaths>; | |||
const { lesson, logoLink, navList, title } = Astro.props as Props; | |||
--- | |||
|
|||
<Layout title={title}> | |||
<Layout title={title} description="A TutorialKit interactive lesson"> |
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.
Ideally this should come from the metadata. You can grab it from Astro.props
here and then it needs to be returned as part of generateStaticRoutes
.
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.
@Nemikolh I would advise to improve the collection, rather than generateStaticRoutes
, to allow feeding more data to the "lesson" object.
Because generateStaticRoutes
is specific to this page only and is difficult to reuse if you want to craft a sitemap or an RSS feed. And you still need a way to pass some metadata from your MDX file to feed "generateStaticRoutes". That's why I couldn't come up with a more customizable/elegant solution yet.
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.
Hey! I think I should have provided more pointers, sorry!
Let me explain. So here's what we need to do:
- Add a
metaTagsSchema
to https://github.com/stackblitz/tutorialkit/blob/main/packages/types/src/schemas/common.ts:
export const metaTagsSchema = z.object({
// ideally we would have want to use image from
// https://docs.astro.build/en/guides/images/#images-in-content-collections
// but we can't so we'll need a custom logic (see below)
image: z.string().optional().describe('An image to show on social previews'),
description: z.string().optional().describe('...'),
title: z.string().optional().describe('...'),
})
...
export const webcontainerSchema = commandsSchema.extend({
meta: metaTagsSchema.optional()
.describe('Configure Open graph meta tags for previews on social medias'),
...
});
- Add
'meta'
here:
tutorialkit/packages/astro/src/default/utils/content.ts
Lines 238 to 240 in f49e910
[ | |
'mainCommand', | |
'prepareCommands', |
-
Now we could handle
image
in the same file when the collection is traversed to resolve all the relative path to be fully resolved. However that won't be enough to have them included in the final bundle. Instead for now we can limit ourselves to images residing in thepublic
folder (so starting with/
). -
Finally update
[...slug].astro
to use the newlesson.meta?.<prop>
properties
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.
@Nemikolh right thanks for the insight, I was not sure if this should be implemented in user-land or not but it seems a sound idea to have at least a title, description and image default setting, having a finer grained system (eg you want a different image for OpenGraph and Twitter, or images outsides of /public or even generated via an API) would be worth a different PR I think
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.
I agree! Yeah, let's start with a simple set of settings and move to a finer grained system later 👍
That's a good idea. I think this can be done in a second step. Like @eric-burel said you still want the data like img, title and description to be defined as part of the metadata of a part / chapter / lesson. Especially because you might want to have a common social preview per chapter but not for the entirety of your tutorial which means that the source of the data is better expressed directly in the metadata. So I think if we start with this first, it should be pretty nice for most use cases 👍 |
I have a small doubt on the update I've done in the documentation : I documented the BASE_URL variable, which is used to compute the logo, favicon or public images. Initially I thought this would the absolute URL of the application (you want absolute urls for open graph tags) but I am not sure. It seems to be a variable set by Astro and which might rather be a kind of separate directory, eg to handle a monorepo. Also I get a messy file diff on GitHub, not sure why. I rebased the branch on "upstream/main" where upstram points on "stackblitz/main" before updating the PR today. Perhaps I should have merged instead? |
I still don't exactly get what |
Hey @eric-burel ! Do you think you could rebase this PR on top of the latest version of main? This would help reviewing this. So you do If you want I can also do it for you, but that means that I'll force push to your branch and you'll have to reset it to the origin. Let me know what you prefer 🙌 |
8c972c0
to
df50505
Compare
@Nemikolh done, I think I've messed up at some point and used a "merge" instead. It has fixed the diff 👌 |
I've also updated the logic to access the site absolute URL, I found another doc explaining that |
Looking at the PR again, I would keep the previous usage of A big downside of forcing them to be absolute is that it makes preview deployment with cloudflare or netlify a bit more annoying as they would need a custom build. Regarding your question on what |
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.
Looks pretty good! Good job on this 🌟
Hi, I'll get back at the few last comments, but in the meantime I've opened a proposal for a portal system in Astro: withastro/roadmap#1032 This would make it easier to support any type of tag that lands into I am thinking of alternate links for instance that I will need in AstroPatterns/NextPatterns because I translate similar content for 2 frameworks and may be punished by Google for that if I don't provide alternate links. |
ebc2719
to
a9c633d
Compare
I've removed the absolute path for the favicon though, which indeed was a mistake.
If you don't set Let me know if I got something wrong! |
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.
Looks really good! Good work on this @eric-burel !
Just a few minor nits and it's good to go 🙌
Co-authored-by: Ari Perkkiö <[email protected]>
Co-authored-by: Jòan <[email protected]>
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.
Looks good to me, thanks @eric-burel!
This PR proposes to use the site logo as the default open graph image, and sets up better default meta tags for opengraph and twitter.
(please double check as opengraph display is usually tested on deployed instances, I couldn't test them locally aside from reading them in the browsers element tab)
Btw I don't think the BASE_URL env variable is documented, to have a proper absolute URL for meta tags.