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

fix: always emit SEO title + og:title meta #7362

Merged
merged 4 commits into from
May 27, 2022

Conversation

charleskorn
Copy link
Contributor

@charleskorn charleskorn commented May 6, 2022

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

⚠️ There doesn't seem to be an existing set of tests for PageMetadata and I can't find any examples of testing a component like this in the codebase (React isn't my strong suit). If you can point me to an example of how something like this would be tested, happy to add some tests for this.

Motivation

<meta property="og:title" content="..." /> tags aren't generated for pages using <Layout> without specifying a page title.

ie. <Layout>...</Layout> does not generate a <meta property="og:title" content="..." /> tag for the page, whereas <Layout title="My page">...</Layout> does generate the meta tag.

This causes the page to fail Twitter's Card Validator.

A default <title> tag seems to get rendered elsewhere, but for completeness, I've changed the behaviour for both tags here.

Test Plan

Manual verification with local site.

⚠️ There doesn't seem to be an existing set of tests for PageMetadata and I can't find any examples of testing a component like this in the codebase (React isn't my strong suit). If you can point me to an example of how something like this would be tested, happy to add some tests for this.

Test links

Deploy preview: https://deploy-preview-7362--docusaurus-2.netlify.app/

Related issues/PRs

(none)

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 6, 2022
@netlify
Copy link

netlify bot commented May 6, 2022

[V2]

Name Link
🔨 Latest commit a54511b
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/6290f296bcc3d50008f9851e
😎 Deploy Preview https://deploy-preview-7362--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented May 6, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 68 🟢 100 🟢 100 🟢 100 🟢 90 Report
/docs/installation 🟠 87 🟢 99 🟢 100 🟢 100 🟢 90 Report

@Josh-Cena Josh-Cena added the pr: bug fix This PR fixes a bug in a past release. label May 7, 2022
@Josh-Cena Josh-Cena changed the title fix: page title and og:title meta aren't emitted if falling back to using site title as page title fix: emit title SEO meta when falling back to using site title as page title May 7, 2022
Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

Makes sense to me 👍

@Josh-Cena
Copy link
Collaborator

Actually, I'm not sure 😅 In this case, pageTitle will always be truthy, so the entire pageTitle && check would not be necessary. It's very likely a bug before, but I'd like to get @slorber to confirm that it wasn't working as intended.

@Josh-Cena Josh-Cena mentioned this pull request May 11, 2022
7 tasks
@lkzwc
Copy link

lkzwc commented May 11, 2022

ok. 明白了 验证没啥问题感觉发布吧 🔥 大佬原来是国人呀 哈哈

@lkzwc
Copy link

lkzwc commented May 17, 2022

这个 bug还没合并吗?

@Josh-Cena
Copy link
Collaborator

@lkzwc Please check the discussion above.

Please don't ask about progress—all progress is evident on GitHub.

charleskorn added a commit to batect/batect.dev that referenced this pull request May 18, 2022
@Josh-Cena
Copy link
Collaborator

Josh-Cena commented May 27, 2022

Ah yes @slorber this needs review & merge for today's release as well. We should remove the pageTitle && check altogether if you confirm that this is the correct behavior.

@slorber
Copy link
Collaborator

slorber commented May 27, 2022

What I understand is that a page not using <PageMetadata title="xyz"/> (through the Layout component or not) does not have any og:title being set.

That would have been convenient to give an example URL to make review easier and see the problem in action: https://docusaurus.io/tests/pages/link-tests

Your solution fixes it, and og:title is now Docusaurus (site title): is that the expected behavior?

https://deploy-preview-7362--docusaurus-2.netlify.app/tests/pages/link-tests


Note that <PageMetadata> is a helper component.

It is supposed to be used 0-n times for a given page, and it's possible that multiple usages override each others according to the order/hierarchy (similar to the React-Helmet doc)

I don't want to remove && usage on purpose, otherwise if we had:

<PageMetadata title="xyz"/>
<PageMetadata description="ABC"/>

Then the last component will override the title that was previously set.

So the current solution is not ideal, will look into fixing it in a better way, ensuring og:title is always set

@Josh-Cena
Copy link
Collaborator

Josh-Cena commented May 27, 2022

@slorber If a page is using <Layout> without title, then nothing gets emitted, and it uses the default "title · tagline" title in the core's fallback metadata instead. This breaks the only workaround to get a page without duplicate site title. See #5878

@slorber slorber changed the title fix: emit title SEO meta when falling back to using site title as page title fix: always emit SEO title + og:title meta May 27, 2022
@slorber slorber merged commit 1e18f8f into facebook:main May 27, 2022
@Josh-Cena
Copy link
Collaborator

To summarize our discussion about this:

  1. The title prop of Layout is soft-deprecated now, and future behavior will not be designed around it. We'd rather make sure the behavior of PageMetadata is intuitive.
  2. If we have <PageMetadata title={undefined} />, this should emit nothing, rather than emitting site title as fallback, because otherwise it means page metadata can't compose.
  3. To this end, we'll simply let it gracefully fallback to something more sensible, if there's no title meta set by the theme.

It's not entirely sorted out—we would probably revisit this next week, refactor it, and probably spec it somewhere (not necessarily public documentation; code comment, most likely).

charleskorn added a commit to batect/batect.dev that referenced this pull request May 27, 2022
thadguidry added a commit to thadguidry/db2rest-web that referenced this pull request Apr 23, 2024
- removed siteconfig.title and description from the <Layout> portion as mentioned in facebook/docusaurus#7362
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants