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(v2): Unbreak blog post title by handling title fallback in LayoutHead #4667

Merged
merged 4 commits into from
Apr 22, 2021
Merged

fix(v2): Unbreak blog post title by handling title fallback in LayoutHead #4667

merged 4 commits into from
Apr 22, 2021

Conversation

SamChou19815
Copy link
Contributor

@SamChou19815 SamChou19815 commented Apr 22, 2021

Motivation

Fix #4666.

What caused the problem

The Head component of docusaurus is re-export of Helmet. According to Helmet docs, items rendered later in Head will override the ones rendered earlier. Currently, there are two places where title is changed when blog post is rendered: <Layout> and <Seo>, and <Seo> happens later than <Layout>.

When rendering a blog post page, it starts in BlogPostPage, which has the correct title:

https://github.com/SamChou19815/docusaurus/blob/d21444b6fb03db87796068fd476f1d10af986182/packages/docusaurus-theme-classic/src/theme/BlogPostPage/index.tsx#L24-L29

Then it starts render BlogPostItem, which has a SEO component that will rerender header: https://github.com/SamChou19815/docusaurus/blob/792f4ac6fb8d7e1124f4b9f5151a827929e6dc50/packages/docusaurus-theme-classic/src/theme/BlogPostItem/index.tsx#L99-L102

Here you can see that the title prop is not passed in. Finally, since title prop is not passed, it causes the blog post title to be overridden to only site title according to

https://github.com/SamChou19815/docusaurus/blob/792f4ac6fb8d7e1124f4b9f5151a827929e6dc50/packages/docusaurus-theme-classic/src/theme/Seo/index.tsx#L15-L22

https://github.com/SamChou19815/docusaurus/blob/d21444b6fb03db87796068fd476f1d10af986182/packages/docusaurus-theme-common/src/utils/generalUtils.ts#L10-L14

My fix is simple: adding the title prop. The practice seems to be consistently with the rest of the codebase, since every other occurrences of Seo will pass in the title prop

Using the advice from @slorber, I reverted the change in #4600 that makes the SEO component handle title fallback. Instead, the fallback is moved to LayoutHead and the SEO fallback will only change title if the passed in title prop is not null.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Goto /blog/2021/03/09/releasing-docusaurus-i18n on preview site, and the title is correct again.

Related PRs

N/A

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 22, 2021
@netlify
Copy link

netlify bot commented Apr 22, 2021

@github-actions
Copy link

github-actions bot commented Apr 22, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 86
🟢 Accessibility 96
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-4667--docusaurus-2.netlify.app/

@slorber
Copy link
Collaborator

slorber commented Apr 22, 2021

Thanks

Unfortunately, I don't think it is the good fix, because now any page displaying a blog post (including the blog index) will have a title.

I think SEO metadata should always be applied at the page component level, and never on a reusable component such as the BlogPostItem.

The problem is that we recently added a fallback to the site title in Seo. @lex111 I think it was not a good thing: instead we should probably apply the fallback title directly in Layout, and only apply the title provided by the user (if provided) in the Seo component, to override the default site title.

Related to this fix: #4600 : I think we should revert it and move the default title in Layout.

@SamChou19815 SamChou19815 changed the title fix(v2): Unbreak blog post title fix(v2): Unbreak blog post title by handling title fallback in LayoutHead Apr 22, 2021
@slorber
Copy link
Collaborator

slorber commented Apr 22, 2021

thanks, that seems to work fine now

@slorber slorber merged commit 2dcbd50 into facebook:master Apr 22, 2021
@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Apr 22, 2021
@SamChou19815 SamChou19815 deleted the unbreak-blog-post-title branch April 22, 2021 17:32
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.

Title not showing correctly for blogs
3 participants