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): fix contentTitle issues when markdown h1 title contains code blocks #4882

Merged
merged 11 commits into from
Jun 3, 2021

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Jun 1, 2021

Motivation

We should support using inline code blocks and other fancy MD syntax in top-level titles:

---
id: docusaurus.config.js
description: API reference for Docusaurus configuration file.
slug: /docusaurus.config.js
---

# `docusaurus.config.js`

This is not the case, as currently reported by @markerikson: #4868 (comment)

This is because titles are extracted as plain string, and then added back at the top of the document as separate JSX.

I think it would be better to leave h1 title as part of the MDX content in the first place, so that any MD syntax remains effective.

@lex111 I tried to make this retro compatible with the existing CSS, it looks fine to me, let me know if you see any problem with this new approach.

Some possible side-effects:

  • the heading/h1 title is now under the .markdown class
  • if a doc has multiple h1 titles (not recommended!), they'll get the same formatting as the top-level title

Have you read the Contributing Guidelines on pull requests?

(Write your answer here.)

Test Plan

tests + preview + dogfood

Related PRs

reduxjs/react-redux#1725

@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Jun 1, 2021
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 1, 2021
const Heading = (Tag: HeadingType): ((props: Props) => JSX.Element) =>
type HeadingComponent = (props: Props) => JSX.Element;

export const MainHeading: HeadingComponent = function MainHeading({...props}) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For h1 found in MDX we don't add anchors.

This is a potential side-effect for a minority of users but using multiple h1 in a doc is not something they should do in the first place

description: API reference for Docusaurus configuration file.
slug: /docusaurus.config.js
---

# `docusaurus.config.js`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

dogfood on our site

@github-actions
Copy link

github-actions bot commented Jun 1, 2021

Size Change: -37 B (0%)

Total Size: 621 kB

Filename Size Change
website/build/assets/css/styles.********.css 88.3 kB -37 B (0%)
ℹ️ View Unchanged
Filename Size Change
website/build/assets/js/main.********.js 443 kB 0 B
website/build/blog/2017/12/14/introducing-docusaurus/index.html 62.3 kB 0 B
website/build/docs/introduction/index.html 235 B 0 B
website/build/index.html 27.1 kB 0 B

compressed-size-action

@netlify
Copy link

netlify bot commented Jun 1, 2021

✔️ [V1]

🔨 Explore the source changes: dbe52b1

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-1/deploys/60b8ec2778d6850007300cf3

😎 Browse the preview: https://deploy-preview-4882--docusaurus-1.netlify.app

@netlify
Copy link

netlify bot commented Jun 1, 2021

✔️ [V2]

🔨 Explore the source changes: dbe52b1

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/60b8ec27b2b097000af49add

😎 Browse the preview: https://deploy-preview-4882--docusaurus-2.netlify.app

@lex111
Copy link
Contributor

lex111 commented Jun 2, 2021

It would be nice to keep the header element as direct child in article, rather than put it within markdown div. Moreover, it makes margin after h1 heading bigger than it was before.

Before After
image image

Also, in the final HTML markup for heading h1 redundant id attribute is set:

image

@slorber
Copy link
Collaborator Author

slorber commented Jun 2, 2021

It would be nice to keep the header element as direct child in article, rather than put it within markdown div.

What are the advantages of having it as a direct child?

Moreover, it makes margin after h1 heading bigger than it was before.

That seems fixable


Will think about it, I have another idea.

@slorber
Copy link
Collaborator Author

slorber commented Jun 2, 2021

@lex111 , before the recent changes regarding markdown title extraction, we already had h1 under markdown.

For example, here's what a React-Redux used to look like before upgrading:
https://deploy-preview-1705--react-redux-docs.netlify.app/api/provider

I think it makes sense that an h1 in markdown appears under the markdown class and not above, and we should restore the previous behavior.

We would have to handle 2 cases:

  • when the title is artificially prepended above div.markdown
  • when the title is in markdown (like here), inside div.markdown and eventually contains md syntax like inline code blocks

We should make them look the same

I'm not sure it's a problem in terms of HTML markup/accessibility?

We used to have those 2 cases before anyway, and it has never been reported as a problem. Not even sure the CSS was 100% equivalent, but quite close so users didn't notice. The most visible difference is that a h1 in markdown had an anchor (cf https://deploy-preview-1705--react-redux-docs.netlify.app/api/provider)

image

So, my suggestion would be to:

  • restore previous behavior, handle 2 cases (title inside markdown / above markdown)
  • make headings look the same in both cases, despite different markup (no anchor, same paddings/margins...)
  • make it unnecessary to use hide_title: true frontmatter in case there's a h1 at top of markdown (unlike before: it was required to avoid a duplicate title issue)

@lex111
Copy link
Contributor

lex111 commented Jun 2, 2021

I like the idea of clear separation of header and body doc item as it used to be, it's kind of best practice. That's why I would like to keep this markup. Then we will not need any additional efforts. This can be achieved, isn't it?

It would also just be nice to have consistency in code, since right now, depending on how first-level heading was specified (via frontmatter or Markdown syntax), our final HTML code (markup) differs.

Markdown Frontmatter
image image

@slorber
Copy link
Collaborator Author

slorber commented Jun 2, 2021

I like the idea of clear separation of header and body doc item

I don't have too much opinion about making it separate or not, but feel it's more complicated to make it separate particularly if

as it used to be

Actually, it didn't use to be like that. Before #4485, a # title in md was under div.markdown element, not above, so I'm just suggesting here to restore how it actually used to be before that PR

it's kind of best practice.

Any ref?

I'm seeing other frameworks like Vuepress or MkDocs also putting the h1 inside the content wrapper at the same level as other elements:
https://vuepress.vuejs.org/guide/#introduction
https://www.mkdocs.org/user-guide/writing-your-docs/

That's why I would like to keep this markup. Then we will not need any additional efforts. This can be achieved, isn't it?
It would also just be nice to have consistency in code, since right now, depending on how first-level heading was specified (via frontmatter or Markdown syntax), our final HTML code (markup) differs.

I agree that having to handle 2 distinct markup cases is not ideal

We have 3 choices:

    1. handle the 2 cases: it's what it used to be until PR was merged
    1. always put h1 inside div.markdown: would be quite simple but you don't seem to like the idea
    1. always put h1 above div.markdown: this feels complex to do right without loosing the ability to use md syntax in title (we would need to parse the title as a standalone MDX document?)

For now I'm going to move on, consider #4485 introduced a regression and restore 1) the previous behavior.

Most users don't use # title in markdown docs yet, and those who do never complained about the different markup

Even if it creates 2 separate cases (like before), at least it fixes the issue of unability to use inline code blocks, which looks more important to users than having a perfect homogeneous markup.

If you are willing to use solution 2), or can make a technical proposition for 3) without loosing the ability to use inline code blocks (looks not easy), then we could improve the markup and have only 1 case left. That can be done in another enhancement PR.

@lex111
Copy link
Contributor

lex111 commented Jun 2, 2021

Alright, let us choose the path of least resistance then -- always put h1 inside div.markdown.

@github-actions
Copy link

github-actions bot commented Jun 3, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 58
🟢 Accessibility 97
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

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

@slorber slorber marked this pull request as ready for review June 3, 2021 15:41
@slorber slorber requested a review from lex111 as a code owner June 3, 2021 15:41
@slorber slorber merged commit 5780679 into master Jun 3, 2021
@slorber slorber deleted the slorber/fix-contentTitle-issues branch August 17, 2021 17:49
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.

3 participants