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

nodejs-zh-TW: Improve translation #1727

Closed
wants to merge 3 commits into from
Closed

Conversation

osk2
Copy link
Contributor

@osk2 osk2 commented Jul 10, 2018

I made some improvements as mentioned in #1719

  1. Remove unnecessary quotation mark
  2. Remove unnecessary translation

Copy link
Contributor

@fhemberger fhemberger left a comment

Choose a reason for hiding this comment

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

Not sure about removing the quotation marks. Didn't check: Is this valid YAML having two colons? Does the site build?

+1 for removing last sentence!

@ghost
Copy link

ghost commented Jul 10, 2018

Please make sure that your second ":" is in Chinese mode instead of English one., or there's no spaces between your : and the 2nd Chinese character. Otherwises the quotation marks should be added. I've had a test in my local machine by running npm run test after npm run build.

Compare with each other:

  1. The : symbol in the Chinese mode, the color is yellow, which means this is of a string type, and you don't need the double quotation marks (Chinese version should match the Chinese symbol double quotations, recommanded!):
    image

The English double quotation marks with NO spaces between 2nd Chinese character should be also OK:
image

The result is:
image

But YOU CANNOT USE AN ENGLISH MODE'S double quotations with a space like this, notice the color is changed for :
image

And you're getting WRONG page like this following:
image

Copy link
Member

@PeterDaveHello PeterDaveHello left a comment

Choose a reason for hiding this comment

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

The "Merge" commits should be removed.

@osk2
Copy link
Contributor Author

osk2 commented Jul 10, 2018

It passed tests and looks no difference since I use fullwidth colon. But I will add quotation mark back to avoid confusion.

@PeterDaveHello oops, I'll open another PR soon.

@osk2 osk2 closed this Jul 10, 2018
@osk2 osk2 deleted the translation branch July 10, 2018 07:30
@ghost
Copy link

ghost commented Jul 10, 2018

@osk2 :BTW,In fact you can remove Merge with the help of "Reset and Rebase" and re-submit instead of closing that :)

@fhemberger
Copy link
Contributor

We'll squash and merge the commits anyway, so no worries about that.

@osk2
Copy link
Contributor Author

osk2 commented Jul 10, 2018

@Maledong Thanks for the tip, I'll try next time 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants