Skip to content
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

Merged
merged 7 commits into from
Nov 26, 2020
Merged

feat: frontmatter global and computed global docs #152

merged 7 commits into from
Nov 26, 2020

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Nov 25, 2020

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:

  • pages

removed props in $page

  • "regularPath": "/guide/global-computed.html"
  • "key": "v-d4cbeb69eff3d"
  • "path": "/guide/global-computed.html"

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.

@patak-dev patak-dev changed the title feat: frontmatter global and feat: frontmatter global and computed global docs Nov 25, 2020
@kiaking kiaking added the enhancement New feature or request label Nov 25, 2020
Copy link
Member

@kiaking kiaking left a 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/frontmatter.md Outdated Show resolved Hide resolved
}
```

## \$theme
Copy link
Member

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.

Copy link
Member Author

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.

@patak-dev
Copy link
Member Author

The name $siteByLocale would have helped me better understand its purpose, +1 for that change. Thanks for the detailed explanation 🙌

About $description, $title, etc, I understand your point. The only thing that makes me want to have $title for example is that it may be good to have a way to refer to the real title and not the $frontmatter.title without having to repeat that logic in user code (so having there the fallback logic from frontmatter to heading to the main config, if I interpret correctly why they are useful in vuepress).
On the other hand, I think it is important to have description as a predefined variable in frontmatter so users can easily overwrite the description for a given page.

It looks like we have an issue with the \ escaping char in the sidebar. I'll add an issue for this one, as it is not related to the PR
image

@kiaking
Copy link
Member

kiaking commented Nov 25, 2020

Thanks for the fixes 🙌 Ahhh, you're right. $title and $description does seem to make sense. So that you don't have to do:

{{ $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.

It looks like we have an issue with the \ escaping char in the sidebar. I'll add an issue for this one, as it is not related to the PR

Thanks! Agreed 👍

@patak-dev
Copy link
Member Author

I am checking $title and $description and looks like we will have:

    $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 router.route.data.description equal to descriptionFromFrontmatter(router.route.data.frontmatter) in the same way that we have router.route.data.title equal to inferTitle:

    // 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 👍

@kiaking
Copy link
Member

kiaking commented Nov 26, 2020

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 resolveDescription?

    // 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 inferDescription would still make sense 😳

BTW, for both title and description, we should use siteByRoute, to get correct value in multi locale env.

return router.route.data.title || siteDataByRouteRef.value.title

@patak-dev
Copy link
Member Author

I went with the fancy inferDescription 😄
I think it still makes sense because maybe we would like to support extracting the description from the first paragraph in the same way we do with the header for the title.

getHeadMetaContent(head,name) should probably be moved to another file later.

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

  1. About siteDataByRoute, if $title and $description are using it, it is really tempting to change $site to point to siteDataByRoute instead of siteData. I think that users accessing $site from the markdown, should really be looking for $siteDataByRoute.
    So maybe we should rename to:
    Even in the theme, it should be used everywhere by locale, no? As an example, currently in NavBar $site is being used. I think the users and even internally there will be a lot of i18n because of this if not.

I see now that even for enhanceApp siteData actually means siteDataByRoute

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.

  1. Even internally, I would prefer to force siteData to always refer to the localized version, I think in the Home component there may be an i18n issue now because siteDataByRoute is not being used instead. Maybe something like:
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()
  1. Another idea: pageData.title could actually be already resolved with the fallback instead of being '' . So it is easier for users because $page.title will be equal to $title (same for description). In this case we need to also add:
    // 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 documentTitle logic is used in a few places in the code (useUpdateHead, renderPage) and it will be good to have a single localized source of truth for it.

@kiaking
Copy link
Member

kiaking commented Nov 26, 2020

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 \$site on the sidebar, thinks it's typo and try to make PR, then they find out that it's an existing issue, and you know, we feel bad for them 😅

getHeadMetaContent(head, name) should probably be moved to another file later.

Agree!

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

Yeah we need serious refactoring around siteByRoute. Let's open an issue to discuss this more. I do think having $site return $siteByRoute is nice and we should use siteByRoute mainly in theme files, but again, we should strive to align with VuePress on public API such as $site.

I also think for convenience, I would like to add the real siteByRoute, not by locale but the site config that gets current sidebar config for multiple sidebar configuration.

Anyway, having dedicated issue to discuss it farther would be great.

Another idea: pageData.title could actually be already resolved with the fallback instead of being '' . So it is easier for users because $page.title will be equal to $title (same for description). In this case we need to also add:

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 $site.title).

@kiaking
Copy link
Member

kiaking commented Nov 26, 2020

About #155, seems like just making the titles ## $page working fine. Do we have any specific reason why we want to escape the $...? If not I would say just update \$site etc to $site and move on for now 👀

@patak-dev
Copy link
Member Author

I am looking into the issue. I'll try to send a fix later 👍

@patak-dev
Copy link
Member Author

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.

@kiaking
Copy link
Member

kiaking commented Nov 26, 2020

Oh OK great! I'll merge this one after merging #137. 👍 And good know with deeplyParseHeader 🤝

@kiaking kiaking merged commit c6bdcfb into vuejs:master Nov 26, 2020
@patak-dev
Copy link
Member Author

Just for reference, at the end deeplyParseHeader was already being performed on headers with the markdown plugin extractHeaderPlugin. That is working as expected 👍

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants