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

feat: add admonition type title translations #7556

Merged
merged 6 commits into from
Jun 3, 2022

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Jun 3, 2022

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

Admonition default titles are not translated by default

This leads translations to use translate :::tip as :::tip astuce to get it translated

This is better to have it translated with theme translations by default.

It is also safer as we can exclude :::tip from translated strings in Crowdin, preventing translation mistakes that may lead to failing CI builds.

Also added:

  • admonition stable theme class names
  • minor refactors

Test Plan

Tested locally

CleanShot 2022-06-03 at 15 52 50@2x

Test links

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

Related issues/PRs

#4542

@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Jun 3, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 3, 2022
};

// Legacy aliases, undocumented but kept for retro-compatibility
const aliases = {
secondary: 'note',
important: 'info',
success: 'tip',
warning: 'danger',
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Warning" is used more often than "danger", at least for our docs. Do we really want to not translate it?

Copy link
Collaborator Author

@slorber slorber Jun 3, 2022

Choose a reason for hiding this comment

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

good point, this is a bit annoying to have these aliases and would require duplicating some translation duplication

btw I'm not fan at all of "caution". "warning" is more natural to me.

Do we really need different default labels for both cases?

Is there a good reason to use both on our own website, or is it just historically the case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that "warning" = "danger" and "caution" being another thing is not at all obvious😅 I can't even think of two distinct words for "warning" and "caution" in Chinese. Maybe we can keep it like this, since it... should be better than what we had?

TBH, on Crowdin, I never translate the admonition titles, because using "tip"/"info" is actually more natural than using any Chinese characters in the title. Maybe that's just me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@forresst do you find it useful to have 2 orange admonitions for warning/caution ?

They are currently translated in French as "avertissement" / "attention", but to me we could as well use "attention" for both and it would remain a good translation

Copy link
Contributor

Choose a reason for hiding this comment

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

@slorber You are right. I agree with you. One is enough. "attention" is perfect

Copy link
Collaborator

@Josh-Cena Josh-Cena Jun 3, 2022

Choose a reason for hiding this comment

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

I'm not good at French at all, but I struggle to capture the nuance between "warning" and "caution" in my Chinese translations. "Caution" does sound less severe than "warning". I think "avertissement" is more severe than "attention" (forgive my ignorance if that's not true)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@forresst personally I find "Attention" more usual in French and would use it in both cases

I guess all this is quite subjective 🤪 hopefully it is now customizable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh yes, I'm misunderstanding things now.

Our alias "warning" -> "danger" feels really wrong to me. To me a warning should be yellow/orange.

BTW it looks like I'm not alone to think so

https://v1.vuepress.vuejs.org/guide/markdown.html#custom-containers

https://nextra.vercel.app/themes/docs/callout

https://vitepress.vuejs.org/guide/markdown.html#custom-containers

https://docs.gitbook.com/editing-content/rich-content/with-command-palette#hints-and-callouts

I'll open a dedicated issue, we should probably do like all others, but we need a way to migrate this gracefully or this will be annoying for our users

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's very right. It has always been tricky for me as well. A breaking change is probably worth it, considering :::warning wasn't documented anyway...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

@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.

If someone has translations to suggest, please do so :)

@@ -12,6 +12,11 @@
"theme.NotFound.p2": "Пожалуйста, обратитесь к владельцу сайта, с которого вы перешли на эту ссылку, чтобы сообщить ему, что ссылка не работает.",
"theme.NotFound.title": "Страница не найдена",
"theme.TOCCollapsible.toggleButtonLabel": "Содержание этой страницы",
"theme.admonition.caution": "caution",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cc @lex111 , want to provide translations?

"theme.admonition.danger": "danger",
"theme.admonition.info": "info",
"theme.admonition.note": "note",
"theme.admonition.tip": "tip",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cc @sseraphini , translations to suggest? 😄

"theme.admonition.danger": "danger",
"theme.admonition.info": "info",
"theme.admonition.note": "note",
"theme.admonition.tip": "tip",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cc @koko8829 @leeyspaul , translations to suggest?

"theme.admonition.danger": "danger",
"theme.admonition.info": "info",
"theme.admonition.note": "note",
"theme.admonition.tip": "tip",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cc @massoudmaboudi translations to suggest?

@netlify
Copy link

netlify bot commented Jun 3, 2022

[V2]

Name Link
🔨 Latest commit 5d6bc15
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/629a19a0c0c8c60008755555
😎 Deploy Preview https://deploy-preview-7556--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 settings.

@github-actions
Copy link

github-actions bot commented Jun 3, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 79 🟢 100 🟢 100 🟢 100 🟢 90 Report
/docs/installation 🟠 81 🟢 100 🟢 100 🟢 100 🟢 90 Report

@github-actions
Copy link

github-actions bot commented Jun 3, 2022

Size Change: +72 B (0%)

Total Size: 798 kB

Filename Size Change
website/build/assets/js/main.********.js 600 kB +72 B (0%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 52.6 kB
website/build/assets/css/styles.********.css 106 kB
website/build/index.html 38.9 kB

compressed-size-action

slorber and others added 2 commits June 3, 2022 16:24
Co-authored-by: Joshua Chen <[email protected]>
Co-authored-by: Joshua Chen <[email protected]>
@slorber
Copy link
Collaborator Author

slorber commented Jun 3, 2022

Going to merge now, we'll figure out the details in other PRs if there's anything to change

If you want to submit your translation, just open a PR after modifying your localized file here: https://github.com/facebook/docusaurus/tree/main/packages/docusaurus-theme-translations/locales

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: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants