stackblitz / tutorialkit

TutorialKit by StackBlitz - Create interactive tutorials powered by the WebContainer API
https://tutorialkit.dev
MIT License
504 stars 48 forks source link

fix(astro): better default meta tags #342

Closed eric-burel closed 1 month ago

eric-burel commented 2 months ago

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.

stackblitz[bot] commented 2 months ago

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

AriPerkkio commented 2 months ago

I'll take a look at the code next week, but another idea came into my mind: Should we create new MetaData component that authors could use to define completely custom meta tags? https://tutorialkit.dev/guides/overriding-components/

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 ./src/components/MetaTags.astro you would get access to default meta tags, and you could decide whether to use them or not. And of course add your custom ones there too.

Improving default meta tags is important too.

eric-burel commented 2 months ago

Definitely, I've thought about a Metadata component but then the question is how to pass the settings from a lesson to this component Maybe using the lesson slug as a unique id This also joins the question of accessing lesson metadata from the content collection to generate tables of content rss feeds etc.

Nemikolh commented 2 months ago

I'll take a look at the code next week, but another idea came into my mind: Should we create new MetaData component that authors could use to define completely custom meta tags?

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 :+1:

eric-burel commented 1 month ago

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. But I am not sure, wha'ts your opinion on this @Nemikolh ? Update: it seems that we might want to use Astro.site in this scenario but I still don't totally grasp "BASE_URL", if we should use both to compute the final absolute URL.

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? This page shows a cleaner diff.

eric-burel commented 1 month ago

I still don't exactly get what BASE_URL is, but I figured that Astro.site is what I want for absolute URLs, I've pushed an update in this direction. It's not super elegant but the SEO tags are at least correct.

Nemikolh commented 1 month ago

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 git fetch <stackblitz-tk-remote> and then git rebase <stackblitz-tk-remote>/main. Trying this, you should only get a single conflict on packages/astro/src/default/layouts/Layout.astro.

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 :raised_hands:

eric-burel commented 1 month ago

@Nemikolh done, I think I've messed up at some point and used a "merge" instead. It has fixed the diff :ok_hand:

eric-burel commented 1 month ago

I've also updated the logic to access the site absolute URL, I found another doc explaining that Astro.site will feed the import.meta.env.SITE env variable for consumption in JS files https://docs.astro.build/en/guides/environment-variables/ Ideally this would be set to "http://localhost:4321" in dev but I am not sure how to reliably get the right port. Astro.url automatically has an absolute URL in development but it's not the right solution, because it's based on the current HTTP request to get a full path while we just want the website root URL so the site value.

Nemikolh commented 1 month ago

Looking at the PR again, I would keep the previous usage of BASE_URL in place of site for resources loaded by the browser (so everything that is not OG or Twitter related) as those links can be relative and don't have to be absolute.

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 BASE_URL is, see the docs on base.

eric-burel commented 1 month ago

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: https://github.com/withastro/roadmap/discussions/1032

This would make it easier to support any type of tag that lands into <head> without having to support a full-fledged intermediate JSON représentation that fits into the YAML frontmatter. Not that it's impossible, it's Next.js approach with the metadata object, but maybe more work than it should at framework-level for something that could be done in user-land with good old HTML.

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.

eric-burel commented 1 month ago

Looking at the PR again, I would keep the previous usage of BASE_URL in place of site for resources loaded by the browser (so everything that is not OG or Twitter related) as those links can be relative and don't have to be absolute.

BASE_URL is still there, and optionally the absolute option can be used to also append the whole website URL via Astro.site. site is only actually used when using the logo as the default ogImage, or passing your own, though I think having it as an option in readPublicAsset may be useful.

I've removed the absolute path for the favicon though, which indeed was a mistake.

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.

If you don't set Astro.site you'll just come up with a warning. I think you are expected to use an env variable in astro.config to handle these cases? Eg Vercel preview URLs.

Let me know if I got something wrong!