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: improve how to use <details> #10173

Merged
merged 1 commit into from
May 30, 2024
Merged

Conversation

tats-u
Copy link
Contributor

@tats-u tats-u commented May 26, 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

The current example for <details> is not ideal one.

  • There are extra <div> elements, which confuse readers (You don't have to use them; they. They're just optional and not mandatory)
  • The contents in <div> confuse the block and inline notations

Block notation:

<div>
  paragraph inside `<div>` element
</div>

Inline notation:

<div>no paragraphs in this line</div>

Test Plan

https://deploy-preview-10173--docusaurus-2.netlify.app/docs/markdown-features#details
https://deploy-preview-10173--docusaurus-2.netlify.app/docs/3.3.2/markdown-features#details

Test links

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

Related issues/PRs

@tats-u tats-u requested review from slorber and Josh-Cena as code owners May 26, 2024 14:50
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 26, 2024
Copy link

netlify bot commented May 26, 2024

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 0072e71
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/66570b016083a500080fcf37
😎 Deploy Preview https://deploy-preview-10173--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 26, 2024

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 61 🟢 98 🟢 96 🟢 100 🟠 88 Report
/docs/installation 🟠 65 🟢 96 🟢 100 🟢 100 🟠 88 Report
/docs/category/getting-started 🟠 75 🟢 100 🟢 100 🟢 90 🟠 88 Report
/blog 🟠 68 🟢 100 🟢 100 🟢 90 🟠 88 Report
/blog/preparing-your-site-for-docusaurus-v3 🟠 62 🟢 96 🟢 100 🟢 100 🟠 88 Report
/blog/tags/release 🟠 67 🟢 100 🟢 100 🟠 80 🟠 88 Report
/blog/tags 🟠 74 🟢 100 🟢 100 🟢 90 🟠 88 Report

@OzakIOne OzakIOne added pr: documentation This PR works on the website or other text documents in the repo. domain: markdown Related to Markdown parsing or syntax labels May 26, 2024
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

I'd be ok to modify the example

However we try to keep the docs short and concise, not to explain every MDX quirk in depth. There's the MDX docs for that.

The warning is way too visible and draw way too much attention to it while most readers shouldn't care much. I'd prefer to remove the warning entirely. If you want to keep it, make it short (max 3 lines) and add a link to relevant MDX docs page. If it's not documented on MDX, make a doc PR to MDX instead. If they don't accept the doc PR, then link to the GitHub issue/discussion explaining the problem. If details must be included, hide them by default in a <details> element to make them less visible.

You see the idea. The goal is not for the docs to become too long because every one document every quirk ever encountered 😅 We care about the experience of reading the docs more than the exhaustiveness of it. Sometimes not having something in the doc is a feature, not a bug.

</details>

</BrowserWindow>
````

:::warning
Copy link
Collaborator

Choose a reason for hiding this comment

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

this section is way too big and visible

@OzakIOne
Copy link
Contributor

image image

Syntax color needs to be fixed

@tats-u
Copy link
Contributor Author

tats-u commented May 29, 2024

Looks like Docusaurus (Prism itself?) doesn't support the highlight for mdx.

@slorber
Copy link
Collaborator

slorber commented May 30, 2024

Thanks, still think it's a bit too much but let's move on

@slorber slorber merged commit 61f71f6 into facebook:main May 30, 2024
9 checks passed
@tats-u tats-u deleted the fix-details branch May 30, 2024 10:15
@tats-u
Copy link
Contributor Author

tats-u commented May 30, 2024

Oh, I've been planning to fix it if you suggest me a changing plan. I think it probably can be a little more shortened with others' help.
I'd appreciated if you could tell me how it should be fixed.
Sorry for falling asleep before leaving comments before the merge.
Of course I'm glad to know this PR is merged and I'll tell you thank you for merging, though...

@tats-u
Copy link
Contributor Author

tats-u commented May 30, 2024

I think I have to send a PR to MDX's document as you pointed out.
This topic is written only in the migration guide to v2, but it's difficult for us to arrive from the top; we have to go to Blog → MDX 2 post before it.

@tats-u
Copy link
Contributor Author

tats-u commented May 30, 2024

Also I've just found I fell asleep before pushing the backport change to 3.3 to GitHub...

tats-u added a commit to tats-u/docusaurus that referenced this pull request May 30, 2024
@tats-u
Copy link
Contributor Author

tats-u commented May 30, 2024

#10180

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA domain: markdown Related to Markdown parsing or syntax 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.

4 participants