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: add admonition for incompatibility with * and ** in Chinese and Japanese in MDX v2+ #9692

Merged
merged 1 commit into from
Jan 18, 2024

Conversation

tats-u
Copy link
Contributor

@tats-u tats-u commented Jan 3, 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

** in **重要的中文。**其他的中文。 and **重要な日本語の文。**別の日本語の文。 will not treated as <strong> due to a regression in CommonMark spec 0.14+.
MDX v2+ strictly follow CM and have this regression.

commonmark/commonmark-spec#650

Test Plan

Test links

https://deploy-preview-9692--docusaurus-2.netlify.app/docs/migration/v3/#other-markdown-incompatibilities

Related issues/PRs

commonmark/commonmark-spec#650

@tats-u tats-u requested a review from slorber as a code owner January 3, 2024 15:25
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jan 3, 2024
@tats-u
Copy link
Contributor Author

tats-u commented Jan 3, 2024

@Josh-Cena I would appreciate if you could provide a Chinese example.

@tats-u tats-u marked this pull request as draft January 3, 2024 15:29
Copy link

netlify bot commented Jan 3, 2024

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 089ebe7
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/65a556063c65150008b9376d
😎 Deploy Preview https://deploy-preview-9692--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.

@tats-u tats-u changed the title docs: add admonition for incompatibility with * and ** in MDX v2+ docs: add admonition for incompatibility with * and ** in Chinese and Japanese in MDX v2+ Jan 3, 2024
Copy link

github-actions bot commented Jan 3, 2024

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 76 🟢 98 🟢 100 🟢 100 🟠 89 Report
/docs/installation 🟠 87 🟢 98 🟢 100 🟢 100 🟠 89 Report
/docs/category/getting-started 🟠 76 🟢 100 🟢 100 🟢 90 🟠 89 Report
/blog 🟠 70 🟢 100 🟢 100 🟢 90 🟠 89 Report
/blog/preparing-your-site-for-docusaurus-v3 🟠 60 🟢 97 🟢 100 🟢 100 🟠 89 Report
/blog/tags/release 🟠 71 🟢 100 🟢 100 🟠 80 🟠 89 Report
/blog/tags 🟠 72 🟢 100 🟢 100 🟢 90 🟠 89 Report

@tats-u tats-u force-pushed the cm-em-strong branch 2 times, most recently from ee9df12 to 73b5e1e Compare January 3, 2024 23:22
@slorber
Copy link
Collaborator

slorber commented Jan 4, 2024

Thanks

@tats-u don't forget to add a link to your docs edit preview in the PR, that makes it much faster to review for others:
https://deploy-preview-9692--docusaurus-2.netlify.app/docs/migration/v3/

This is a long text for a little issue. The goal here is not to document them all exhaustively and overwhelm users that migrate, most of them are not Chinese/japanese.

What about putting most of that content in <details> element, so that only the interested readers will expand it? (or brievly mention the problem, and link to a dedicated GitHub discussion?)

@tats-u
Copy link
Contributor Author

tats-u commented Jan 4, 2024

don't forget to add a link to your docs edit preview in the PR,

I had thought that is not necessary. I have fixed the description and will add ones from an next PR.

a little issue

I doubt this because the official site of Docusaurus is also affected.

https://docusaurus-i18n-staging.netlify.app/ja/docs

image

https://docusaurus.io/zh-CN/docs/next

image

https://docusaurus-i18n-staging.netlify.app/ja/docs/static-assets#in-css

image

Note: Chinese has avoided this by adding an additional space.

https://docusaurus.io/zh-CN/docs/next/static-assets#in-css

image

I believe it is too frequent to be pushed into a detail element. Also it is difficult for most Chinese and Japanese users to understand the trigger condition of this problem by themselves. They will have to gaze into the CM spec, which is not for users, to find out the condition.
I believe there are few Japanese or Japanese people other than me who are sure to find out * and ** affected by this "specification".
This problem affects on relatively smaller numbers of people, but does relatively severely on them.

According to https://www.meilisearch.com/docs/learn/what_is_meilisearch/language, some other languages can be said to be affected:

  • Hebrew
  • Khmer
  • Korean (I think less affected than Chinese or Japanese; it uses space to split clauses)
  • Thai

What about putting most of that content in <details> element, so that only the interested readers will expand it?

Even if it were adopted, at least <details> must be unwrapped by intention in Japanese and Chinese translation.
Also I will try to shorten the description to make it unnecessary to wrap it into <detail>.

@slorber
Copy link
Collaborator

slorber commented Jan 4, 2024

According to meilisearch.com/docs/learn/what_is_meilisearch/language, some other languages can be said to be affected

I'm ok to have a clear and concise description of the problem, including a list of all languages possibly affected, but I'd like to keep it short for others. Maybe at least put most of the examples in details element?

Even if it were adopted, at least

must be unwrapped by intention in Japanese and Chinese translation.

Hmm why not, but is this really necessary? We can just say that those supporting a list of languages should rather pay more attention to the following section.

@tats-u
Copy link
Contributor Author

tats-u commented Jan 7, 2024

@slorber I found that it looks better to use <details> as you have said.
This offending "specification" requires us to understand the detailed conditions but it is too long for the outside of <details> even in Chinese or Japanese.

@tats-u tats-u force-pushed the cm-em-strong branch 2 times, most recently from db0f9a3 to 85ad9a7 Compare January 7, 2024 14:42
@tats-u tats-u marked this pull request as ready for review January 7, 2024 15:00
@tats-u
Copy link
Contributor Author

tats-u commented Jan 16, 2024

@slorber I will use <details> in all languages. I revised the draft. You can review its content again.

@slorber
Copy link
Collaborator

slorber commented Jan 18, 2024

Thanks 👍

I think it's still a bit too verbose, but let's move on

@slorber slorber added the pr: documentation This PR works on the website or other text documents in the repo. label Jan 18, 2024
@slorber slorber merged commit f794559 into facebook:main Jan 18, 2024
9 checks passed
@tats-u tats-u deleted the cm-em-strong branch January 18, 2024 15:33
@tats-u
Copy link
Contributor Author

tats-u commented Jan 18, 2024

@slorber Thank you for merging. I think we need to backport this (and #9471) to the 3.x branch because visitors on docusaurus.io view the content in the latest 3.x release first.

@slorber
Copy link
Collaborator

slorber commented Jan 18, 2024

@slorber Thank you for merging. I think we need to backport this (and #9471) to the 3.x branch because visitors on docusaurus.io view the content in the latest 3.x release first.

Docs backports happen on main in the website/docs/versioned_docs folder. Feel free to open another PR with similar changes.

@tats-u
Copy link
Contributor Author

tats-u commented Jan 18, 2024

That makes sense. Thank you for providing the helpful information.

tats-u added a commit to tats-u/docusaurus that referenced this pull request Jan 29, 2024
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