-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat: frontmatter global and computed global docs #152
Conversation
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.
Thanks a bunch! Looking really good.
I've added few comment regarding that so please have a look 🙏
I think it would be better to align the config to VuePress as much as possible. Ideally, VuePress should be extending the VitePress configs, so VuePress having more config and aliases than VitePress is fine, but it shouldn't have different names on same thing.
So, let's wait for $lang, $description and $title. It's handy yes, but not handy as frontmatter
. It might be nice for VuePress to have them, not sure if we wanna add that much to VitePress.
There is a new $siteByRoute, but I did not document it as it seems it is related to debugging only.
$siteByRoute
is quite neat feature and we use it often in the theme. It's same as $site
, but it contains $site
data depending on the current route. Handy stuff.
For example, if you have following site config.
{
locales: {
'/': { title: 'Hello' },
'/ja/': { title: 'こんにちは' }
}
}
And when you're at https://example.com/ja/
, $siteByRoute
will give you this.
{ title: 'こんにちは' }
That's been said, let's wait adding this one for now. We might wanna change name of it (to maybe something like $siteByLocale
?), or rethink how we can align this data with VuePress, and so on.
docs/guide/global-computed.md
Outdated
} | ||
``` | ||
|
||
## \$theme |
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.
Can we make this $themeConfig
to align with VuePress.
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.
Great that you want to change it, I was going to add a comment about this particular rename because I do not think we gain anything by breaking compat with vuepress with it. I agree that we should align the naming in the two projects if possible.
Thanks for the fixes 🙌 Ahhh, you're right. {{ $frontmatter.title || $site.title }} Hmmmm, good point. It does make sense to add them. Would like to add it to this PR? I'm in.
Thanks! Agreed 👍 |
I am checking $title: {
get() {
return router.route.data.title || siteDataRef.value.title
}
},
$description: {
get() {
return descriptionFromFrontmatter(router.route.data.frontmatter) || siteDataRef.value.description
}
}, With a new utility in util.ts export function descriptionFromHead(head: HeadConfig[]) : string | undefined {
if (!head || !head.length) {
return undefined
}
const metaDescription = head.find(([tag, attrs = {}, innerHTML = '']) => {
return tag === 'meta' && attrs.name === 'description' && attrs.content
})
return metaDescription && metaDescription[1].content
}
export function descriptionFromFrontmatter(frontmatter: Record<string, any>) {
// We can later add frontmatter.description here
return descriptionFromHead(frontmatter.head)
} Maybe we should already add // inject page data
const pageData: PageData = {
title: inferTitle(frontmatter, content),
description: descriptionFromFrontmatter(router.route.data.frontmatter), // <- this one
frontmatter,
headers: data.headers,
relativePath: file.replace(/\\/g, '/'),
lastUpdated
} So the global computes are aligned. If you are ok adding it, I'll add it to the PR 👍 |
Hmmmm good point. I think it makes more sense to add it at server as you mentioned. In that case, maybe the method name could more simple like // inject page data
const pageData: PageData = {
title: inferTitle(frontmatter, content),
description: resolveDescription(frontmatter), // <- this one
frontmatter,
headers: data.headers,
relativePath: file.replace(/\\/g, '/'),
lastUpdated
} Or maybe BTW, for both title and description, we should use return router.route.data.title || siteDataByRouteRef.value.title |
I went with the fancy
I do not think we should do this change in this PR, but I write it down and we can spawn other issues/PRs later
I see now that even for export default {
Layout,
NotFound: () => 'custom 404',
enhanceApp({ app, router, siteData }) {
// `siteData`` is a `ref`` of current site-level metadata. <- this is really siteDataByRoute
}
} So it makes sense for $site to follow suit.
const siteData = useSiteData() // <- this actually returns siteDataByRoute, by using useRoute internally And in the few cases where you do not want to be localized const unlocalizedSiteData = useUnlocalizedSiteData()
// inject page data
const mdTitle = inferTitle(frontmatter, content)
const mdDescription = inferDescription(frontmatter)
const pageData: PageData = {
title: mdTitle || siteData.title,
description: mdDescription || siteData.description,
documentTitle: ( mdTitle ? mdTitle + ` | ` : `` ) + siteData.title,
...
} That |
Thanks a lot great work! 🙌 Though, I would like to fix #155 before merging this PR. It's going to deploy the docs, people see
Agree!
Yeah we need serious refactoring around I also think for convenience, I would like to add the real Anyway, having dedicated issue to discuss it farther would be great.
I'm open for a discussion, but maybe in some case people would want to know if the page title exist or not? There could be a chance where page title is not set, and might wanna do something in that case (set different title than |
About #155, seems like just making the titles |
I am looking into the issue. I'll try to send a fix later 👍 |
Looks like my setup was inserting these escaping chars, you are right. We do not need them. So we should be good now 😄 That being said, we still need to escape the pageData.headers titles using deeplyParseHeader like we do with tittle, I'll look at that in another PR later. |
Oh OK great! I'll merge this one after merging #137. 👍 And good know with |
Just for reference, at the end |
Removed globals: $lang, $localePath, $description, $title
I think it will be good to add $lang, $description and $title as globals (and description as a predefined variable in frontmatter too) to vitepress too.
Renamed global:
$themeConfig -> $theme
removed props in $site:
removed props in $page
Several new props are available, I included them in the json examples in the doc
There is a new $siteByRoute, but I did not document it as it seems it is related to debugging only.