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

docs: backport #10173 to v3.3 + v3.4 & revise the content #10180

Merged
merged 5 commits into from
Jun 21, 2024

Conversation

tats-u
Copy link
Contributor

@tats-u tats-u commented May 30, 2024

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.

Motivation

I fell asleep before pushing the backport change.
#10173 change is available only in Canary.

Also its content (of the info callout) can be more shortened.
A link to the MDX's migration guide can exist.
Any advices and amendents are welcome.

Update: The blank line after <summary>...</summary> is virtually required, or React emits a warning: Warning: validateDOMNesting(...): <summary> cannot appear as a descendant of <p>.

Test Plan

(https://deploy-preview-10180--docusaurus-2.netlify.app/docs/markdown-features#details) (if the content is revised)
https://deploy-preview-10180--docusaurus-2.netlify.app/docs/3.3.2/markdown-features#details

Test links

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

Related issues/PRs

#10173

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 30, 2024
@tats-u tats-u mentioned this pull request May 30, 2024
3 tasks
Copy link

netlify bot commented May 30, 2024

[V2]

Name Link
🔨 Latest commit 7eb56d7
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/667563e7481fe7000835e229
😎 Deploy Preview https://deploy-preview-10180--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 configuration.

Copy link

github-actions bot commented May 30, 2024

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟢 92 🟢 98 🟢 96 🟢 100 🟠 88 Report
/docs/installation 🟠 67 🟢 96 🟢 100 🟢 100 🟠 88 Report
/docs/category/getting-started 🟠 75 🟢 100 🟢 100 🟢 90 🟠 88 Report
/blog 🟠 70 🟢 100 🟢 100 🟢 90 🟠 88 Report
/blog/preparing-your-site-for-docusaurus-v3 🟠 63 🟢 96 🟢 100 🟢 100 🟠 88 Report
/blog/tags/release 🟠 70 🟢 100 🟢 100 🟢 90 🟠 88 Report
/blog/tags 🟠 77 🟢 100 🟢 100 🟢 90 🟠 88 Report

@tats-u tats-u marked this pull request as ready for review May 30, 2024 13:33
@slorber slorber added the pr: documentation This PR works on the website or other text documents in the repo. label May 31, 2024
Comment on lines 233 to 235
- You should not break lines between the starting or ending tag of the `<summary>` element and its content or an extra `<p>` element is inserted.
- Blank lines between the `<summary>` element and the first paragraph of the detailed content are optional in Docusaurus but strongly recommended to ensure MDX portability with other site generators.
- Blank lines between the nested `<details>` element and the paragraphs just around it are optional.
Copy link
Collaborator

@slorber slorber May 31, 2024

Choose a reason for hiding this comment

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

I would just quickly mention something like:

You may want to keep your <summary> on a single line. MDX creates extra HTML <p> paragraphs for line breaks.

And if there's a good mdx doc explaining this, link to it.

It's the responsibility of the user to decide whether or not they want line breaks, it might make sense in some cases 🤷‍♂️

About the portability, maybe that's true but even I am unsure of what you mean here? Do you mean the Docusaurus MDX v2+ code would be incompatible with other systems still using MDX v1? That seems overkill to me to mention that, most of the ecosystem is already using MDX v2+ and those who don't will probably upgrade

Copy link
Contributor Author

@tats-u tats-u Jun 2, 2024

Choose a reason for hiding this comment

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

other systems still using MDX v1?

Intended for not v1 but v2.
In the first place, I've just found React emits the following warning (error by the default behavior of React 18+) in the console in the DevTools: Warning: validateDOMNesting(...): <summary> cannot appear as a descendant of <p>.
I'll reword it as like "You should, or React emits the warning."

You may want to keep your <summary> on a single line. MDX creates extra HTML <p> paragraphs for line breaks.

I'll take this expression for the first item.

if there's a good mdx doc explaining this, link to it.

I think the current best item is https://mdxjs.com/migrating/v2/#jsx.

@tats-u tats-u changed the title docs: backport #10173 to v3.3 (& revise the content) docs: backport #10173 to v3.3 & revise the content Jun 2, 2024
Comment on lines 234 to 235
- You should add a blank line between the `<summary>` element and the first paragraph of the detailed content, or React will emit a warning in the console in the DevTools: `Warning: validateDOMNesting(...): <summary> cannot appear as a descendant of <p>.`
- You can optionally add a blank line between the nested `<details>` element and the paragraphs just around the nested `<details>` to clarify the semantic boundary between the two.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm going to remove that too because this is incorrect

A blank line after summary is not required. In general, the only rule that apply is how MDX creates extra paragraphs on line breaks

These 2 forms do not involve blank lines and are perfectly valid:

<details>
  <summary>hey</summary>
  <>hoo</>
</details>
<details><summary>hey</summary>hoo</details>

Copy link
Contributor Author

@tats-u tats-u Jun 24, 2024

Choose a reason for hiding this comment

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

@slorber

on line breaks

If the paragraph candidate is composed of only inline JSXes (<...>...</...>) and whitespaces (including line breaking), it's not enclosed by a <p>.

<>foo</>bar<>baz</>

↑ will be enclosed by <p> because bar is a bare element.

<>foo</><>bar
baz</><>qux</>
<>quux</>

↑ won't be enclosed by <p> because they consist of 3 inline JSXes and a newline (between <>qux</> and <>quux</>)

<details><summary>hey</summary>hoo</details>

↑The entire line is the paragraph candidate, but all the content is enclosed by the inline JSX, so no paragraph is inserted.

<details>
  <summary>hey</summary>
  <>hoo</>
</details>

↑Its paragraph candidate is (block JSX narrows the range of paragraph candidate):

  <summary>hey</summary>
  <>hoo</>

The entire content of both 2 lines is enclosed by an inline JSX tag (<summary> and <>).
With a notation like <>hoo</>, you can't add another paragraph or a code block there.

<summary>hey</summary>
<>foo
bar
baz</>
<>qux
quux</>

This is also OK because no bare elements (not enclosed by inline JSX)

<summary>hey</summary>
<>foo
bar</>
baz
<>qux
quux</>

baz is bare there, so a paragraph will be inserted.

I feel it's difficult for me to explain it to beginners simply.

@slorber slorber changed the title docs: backport #10173 to v3.3 & revise the content docs: backport #10173 to v3.3 + v3.4 & revise the content Jun 21, 2024
@slorber slorber merged commit 91cef62 into facebook:main Jun 21, 2024
11 checks passed
@tats-u tats-u deleted the fix-details2 branch June 24, 2024 10:48
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: documentation This PR works on the website or other text documents in the repo.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants