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): static phrasing content should be rendered correctly in TOC #1992

Merged
merged 2 commits into from
Nov 16, 2019

Conversation

endiliey
Copy link
Contributor

Motivation

TOC always render as string. Never putting the html markup.

Previously, only the textual content of headings was used to generate the entries in the Table of Contents. This lead to a bug (or feature?) where HTML in headings made entries in the TOC appear with literal HTML.

Example:

## <i>hello</i>

Will be rendered as literal
image

Other example is below. See that the right toc does not render the emphasis, etc

before

mdast has a concept of static phrasing content, which includes all the things that can appear in links. Instead of using just the textual content of a heading, this commit introduces a new behaviour where any static phrasing content, such as HTML, inline code, and emphasis, are copied over into the Table of Contents.

See test plan

Have you read the Contributing Guidelines on pull requests?

yes

Test Plan

After. See that its rendered correctly now
after

  • Newly added test passes
  • Old test still passing

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 14, 2019
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Nov 14, 2019

Deploy preview for docusaurus-2 ready!

Built with commit 9ae161f

https://deploy-preview-1992--docusaurus-2.netlify.com

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Nov 14, 2019

Deploy preview for docusaurus-preview ready!

Built with commit 9ae161f

https://deploy-preview-1992--docusaurus-preview.netlify.com

@endiliey endiliey added the pr: bug fix This PR fixes a bug in a past release. label Nov 14, 2019
Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Wonder if we rather just render the value to HTML and pass it directly.

@@ -37,7 +37,7 @@ function Headings({headings, isChild}) {
{headings.map(heading => (
<li key={heading.id}>
<a href={`#${heading.id}`} className={LINK_CLASS_NAME}>
{heading.value}
<div dangerouslySetInnerHTML={{__html: heading.value}} />
Copy link
Contributor

Choose a reason for hiding this comment

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

This adds an extra <div> element. Why don't do dangerouslySetInnerHTML on the <a>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. I am currently away so will update after that then

@endiliey endiliey merged commit a4585ec into master Nov 16, 2019
@endiliey endiliey deleted the endi/doctoc branch November 16, 2019 10:41
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.

4 participants