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 escape pipe in Markdown tables #5539

Merged
merged 4 commits into from
Sep 8, 2021
Merged

docs: improve escape pipe in Markdown tables #5539

merged 4 commits into from
Sep 8, 2021

Conversation

forresst
Copy link
Contributor

@forresst forresst commented Sep 8, 2021

From a remark on Crowdin:

The value for the Type entry in the plugin-content-blog page is strange.
In English documents, the Type item value is normally displayed.
ex) editUrl: string | EditUrlFunction
However, in French documentation, the Type item value is shown as an unknown code.
ex) editUrl:!!crwdBlockTags_249_sgaTkcolBdwrc!!
ex) blogSidebarCount: !!crwdBlockTags_250_sgaTkcolBdwrc!!

I think that the string <code> 'light' &#124; 'dark' </code> is blocked by Crowdin because of the pipe escape

This is a test to see if the other way to escape a pipe in a markdown table could solve the problem.

Motivation

First, see if the preview of this page works well in Docusaurus.

In a second time, if the first point works, see if it unblocks the problem in Crowdin

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!)

Related PRs

None

From a remark on Crowdin:
> The value for the Type entry in the plugin-content-blog page is strange.
> In English documents, the Type item value is normally displayed.
> ex) editUrl: string | EditUrlFunction
> However, in French documentation, the Type item value is shown as an unknown code.
> ex) editUrl:!!crwdBlockTags_249_sgaTkcolBdwrc!!
> ex) blogSidebarCount: !!crwdBlockTags_250_sgaTkcolBdwrc!!

This is a test to see if the other way to escape a pipe in a markdown table could solve the problem.
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Sep 8, 2021
@netlify
Copy link

netlify bot commented Sep 8, 2021

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: 15d7ad0

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

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

@github-actions
Copy link

github-actions bot commented Sep 8, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 95
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

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

@forresst
Copy link
Contributor Author

forresst commented Sep 8, 2021

The other way to escape a pipe in a markdown table works:

image

Is it possible to merge this PR to see if it can solve the problem in Crowdin ?

@Josh-Cena
Copy link
Collaborator

Can we also fix this in other places? In plugin-content-blog and plugin-content-docs

@forresst
Copy link
Contributor Author

forresst commented Sep 8, 2021

Can we also fix this in other places? In plugin-content-blog and plugin-content-docs

@Josh-Cena That's what I was thinking of doing in a second step, but I'd like to check that it fixes the problem in Crowdin first

For now, here is the problem in crowdin:
image

@Josh-Cena
Copy link
Collaborator

Yes, but even if it doesn't, it improves readability (I didn't like the HTML entities either but didn't even think <code> \| as a possibility), and things are currently broken anyways, so it will be a gain with no loss

@forresst
Copy link
Contributor Author

forresst commented Sep 8, 2021

@Josh-Cena Ok I will modify all the docs

@forresst forresst changed the title Test escape pipe in Markdown table WIP: Test escape pipe in Markdown table Sep 8, 2021
@lex111
Copy link
Contributor

lex111 commented Sep 8, 2021

@forresst looks good to me, can I merge it?

@forresst forresst changed the title WIP: Test escape pipe in Markdown table Improve escape pipe in Markdown table Sep 8, 2021
@forresst
Copy link
Contributor Author

forresst commented Sep 8, 2021

@lex111 Perfect! Thanks! You can merge it!

@lex111 lex111 added documentation The issue is related to the documentation of Docusaurus pr: documentation This PR works on the website or other text documents in the repo. and removed documentation The issue is related to the documentation of Docusaurus labels Sep 8, 2021
@lex111 lex111 changed the title Improve escape pipe in Markdown table docs: improve escape pipe in Markdown tables Sep 8, 2021
@lex111 lex111 merged commit 02eee61 into facebook:main Sep 8, 2021
@lex111
Copy link
Contributor

lex111 commented Sep 8, 2021

@forresst thanks!

@forresst forresst deleted the patch-1 branch September 8, 2021 12:12
@forresst
Copy link
Contributor Author

forresst commented Sep 8, 2021

Unfortunately, this still does not solve the problem in Crowdin 😢

@Josh-Cena
Copy link
Collaborator

My second guess is the <code> tag inside markdown table, is there a way to test Crowdin without updating the website? 🤔

@forresst
Copy link
Contributor Author

forresst commented Sep 8, 2021

@Josh-Cena I think you are right. the tag can perhaps be problematic in a markdown table.

I don't know if it is possible to test it directly in Crowdin. An opinion @slorber ?

@lex111
Copy link
Contributor

lex111 commented Sep 8, 2021

I'm not sure if Crowdin has a playground or something like that (FYI slorber on vacation).
Maybe we should change Type column to Possible Values, where we list comma-separated acceptable values?

Name Possible Values Default Description
defaultMode 'light', 'dark' 'light' The color mode when user first visits the site.

@Josh-Cena
Copy link
Collaborator

@lex111 The original intent was to have the notation resemble TS—we have Item[], number | 'ALL', string | EditUrlFunction, etc., not sure if "possible values" would break the consistency

@lex111
Copy link
Contributor

lex111 commented Sep 9, 2021

I misspelled, of course we are talking about types, not values. And it is unlikely that such change would worsen the understanding about Possible Types of fields. In our situation it would be a good compromise, so let's give it a try. We could even use just white space as a separator instead of comma.

@slorber
Copy link
Collaborator

slorber commented Sep 22, 2021

Hey, created a dedicated issue and will ping Crowdin support about it: #5602

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.

None yet

5 participants