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

chore(theme-translations): complete ko translations #7762

Merged
merged 8 commits into from
Jul 12, 2022

Conversation

anaclumos
Copy link
Contributor

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

This improves Korean Translations for Algolia Search.

Test Plan

Test links

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

Related issues/PRs

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jul 11, 2022
@Josh-Cena Josh-Cena changed the title Improve Algolia Search Korean Translations chore(theme-translations): complete ko translations Jul 11, 2022
@Josh-Cena Josh-Cena added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Jul 11, 2022
@netlify
Copy link

netlify bot commented Jul 11, 2022

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 9ea7395
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/62cc041b4ec86900094a46c3
😎 Deploy Preview https://deploy-preview-7762--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 Jul 11, 2022

⚡️ Lighthouse report for the deploy preview of this PR

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

@netlify
Copy link

netlify bot commented Jul 11, 2022

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit aa3e5d3
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/62cc1056034dbb0008c3c22c
😎 Deploy Preview https://deploy-preview-7762--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.

@Josh-Cena
Copy link
Collaborator

Pinging @revi @winterlood @HyunseungLee-Travis @koko8829

(Really appreciate it btw—you guys are making ko one of the most well-maintained locales, both here and on Crowdin.)

@anaclumos
Copy link
Contributor Author

I added some screenshots to help the peer checkings 😌
리뷰하시기 편하시라고 스크린샷 첨부해요.

image

image

image

"theme.SearchModal.footer.closeText": "로 종료",
"theme.SearchModal.footer.navigateDownKeyAriaLabel": "화살표 아래 키",
"theme.SearchModal.footer.navigateText": "로 이동",
"theme.SearchModal.footer.navigateUpKeyAriaLabel": "화살표 위 키",
"theme.SearchModal.footer.searchByText": "Search by",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this is fine?

I did translate it for zh btw. I understand the order of words may be a bit weird in Asian languages. I basically translated it as "Search provider".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This actually makes more sense to me, from the keyboard glyph on the modal.

If these translations will only be used for the below screenshot, it suits well.

If it’s used somewhere else, then I might need to review it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, "Search by algolia" is only in this one place AFAICT. It's fine if you don't want to translate it. I did translate it for zh:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait, do you mean “Search By”? Well then it’s whatsoever. I think it wouldn’t really matter if that remains in English. I didn’t think of any better alternatives.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it's awkward in Chinese as well. I wouldn't mind since it's not informative anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we could do 검색 제공.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the context, I misunderstood your first comment. I have added translation keys for Search by.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is fine either way, translated or untranslated.

@revi
Copy link
Contributor

revi commented Jul 11, 2022

Translation LGTM. (Code-Review) +1.

Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

Thank you!

@Josh-Cena
Copy link
Collaborator

Josh-Cena commented Jul 11, 2022

Oh BTW, @koko8829 has already submitted translations for these on Crowdin haha: https://crowdin.com/translate/docusaurus-v2/642/en-ko?filter=basic&value=0 I can't read Hangul but I assume you've compared the two versions? (Can be viewed on https://docusaurus.io/ko/)

@anaclumos
Copy link
Contributor Author

@Josh-Cena, I prefer my translations, but I would love to discuss it with @koko8829.

@koko8829
Copy link
Contributor

@Josh-Cena, I prefer my translations, but I would love to discuss it with @koko8829.

I like your translation.
I especially like the idea of "theme.SearchModal.footer.closeText": "로 종료".

@Josh-Cena
Copy link
Collaborator

@koko8829 So you would confirm this PR is good to merge? Would one of you update the translations on Crowdin, so when this gets merged, the production build will have the new translations?

@koko8829
Copy link
Contributor

koko8829 commented Jul 12, 2022

@koko8829 So you would confirm this PR is good to merge? Would one of you update the translations on Crowdin, so when this gets merged, the production build will have the new translations?

Translation LGTM. I will update the translations on Crowdin.

@Josh-Cena
Copy link
Collaborator

Thanks, please let me know when you're done so we can merge this and do a production build.

@koko8829
Copy link
Contributor

koko8829 commented Jul 12, 2022

Thanks, please let me know when you're done so we can merge this and do a production build.

The update has been completed.

@Josh-Cena
Copy link
Collaborator

Nice, let's merge then. Thanks again @anaclumos @revi @koko8829!

This was referenced May 13, 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: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants