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($core): add canonical link to frontmatter #2658

Merged

Conversation

d-pollard
Copy link
Collaborator

@d-pollard d-pollard commented Oct 12, 2020

Summary

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:

If changing the UI of default theme, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature
  • Related documents have been updated
  • Related tests have been updated

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

This feature introduces the ability to specify canonical in the frontmatter section on a page:

---
canonical: https://google.com
---

Which will then render the proper head HTML tag, and also keep it updated when the page changes

@d-pollard
Copy link
Collaborator Author

Partial resolution to #2153

@adamdehaven
Copy link
Contributor

@d-pollard I can confirm (with your latest addition) that the frontmatter.canonical property works, and is also exposed to the extendPageData Options API, meaning it can be dynamically injected into the frontmatter 🎉

This should definitely roll out ASAP - you're awesome!

@adamdehaven
Copy link
Contributor

@d-pollard don't forget: the docs would need to be updated for this PR as well.

Now you've got me thinking about injecting structured data in a similar way. It also would need to be updated on each route change. I wonder if I'm the only person that would like a hook in the Options API that would allow for injecting (via extendPageData) custom content/tags. In my use-case, I'd want to inject a <script> tag with JSON into the <head>, something like this:

const structuredData = {
    '@context': 'https://schema.org/',
    '@type': 'WebSite',
    name: this.meta_data.title + (this.$page.frontmatter.subtitle ? ' | ' + this.$page.frontmatter.subtitle : '') || null,
    description: this.meta_data.description || null,
    url: frontmatter.canonical,
    image: {
        '@type': 'ImageObject',
        url: this.$site.themeConfig.domain + this.$site.themeConfig.defaultImage,
    },
    '@id': frontmatter.canonical,
}
<script type="application/ld+json">{{ JSON.stringify(structuredData, null, 4) }}</script>

I'm currently doing this with a custom component; however, it is technically rendered after the DOM, meaning it isn't available when the page is scraped (before JavaScript is enabled).

@d-pollard
Copy link
Collaborator Author

Yeah we could definitely look into a custom hook

Typically I write the feature in it's entirety then open another PR with docs code so that if the version changes, the feature code is still introduced and I can update the docs to reflect the proper version

@d-pollard
Copy link
Collaborator Author

d-pollard commented Oct 13, 2020

Like in this case, I have reservations about using just the keyword canonical so will probably change it to be more idiomatic (thinking canonicalLink to reflect current codebase practices)

@adamdehaven
Copy link
Contributor

adamdehaven commented Oct 13, 2020

@d-pollard I almost suggested a change regarding the canonical keyword. I post content on dev.to and they actually use the YAML keyword canonical_url instead, which, I think would be good; however, since other keywords are camelCase, maybe canonicalUrl instead?

I lean towards url rather than link because link seems to reference more of the HTML <link> element itself, whereas url references the value, which is what the user would be providing in the frontmatter.

Edit: ... and definitely let me know if that custom hook for injecting other content comes about 💯

@d-pollard
Copy link
Collaborator Author

Yeah that makes sense, I'll make the changes probably tomorrow, and then document the feature once it's all merged in.

Doing this makes me wonder if I could also use the same pattern to introduce vue-meta in a way that's backwards compatible

@d-pollard
Copy link
Collaborator Author

I fibbed; I've updated the PR with the new terms; used canonicalLink for the SSR injection to reflect the HTML element it's injecting, and I've used canonicalUrl for the frontmatter value to reflect the actual use case

@adamdehaven
Copy link
Contributor

That all looks good to me.

@d-pollard d-pollard requested a review from bencodezen October 13, 2020 14:02
Copy link
Member

@bencodezen bencodezen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks epic @d-pollard! Could we just add a small note inside of the docs to surface that this is now available?

@d-pollard d-pollard requested a review from bencodezen October 13, 2020 14:18
Copy link
Member

@bencodezen bencodezen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the collab effort @d-pollard and @adamdehaven!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants