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

Add translation for zh-tw #567

Merged
merged 9 commits into from
Jun 11, 2018
Merged

Add translation for zh-tw #567

merged 9 commits into from
Jun 11, 2018

Conversation

dxball
Copy link
Contributor

@dxball dxball commented May 15, 2018

No description provided.

@bastimeyer
Copy link
Member

Thanks for submitting!
Can you rebase to master please? There was an issue related to MomentJS and its used locale in the test coverage run which I noticed earlier today while translating to German. I don't know why it's only failing in the coverage run and not in the regular test run, but I fixed the failing test, so that your PR should be working once rebased to master. Thanks again!

@dxball
Copy link
Contributor Author

dxball commented May 15, 2018

I've rebase to master, but the ci still failed 😭

@bastimeyer
Copy link
Member

Yeah, the i18n system is still a bit rough around the edges. One test is failing only on Windows, which is once again a bit surprising. I'll have a look at it...

@bastimeyer
Copy link
Member

bastimeyer commented May 15, 2018

Let's ignore the failing test for now. It's unrelated to the pull request or translations in general, anyway. Just a poorly written test that's failing on the slow Appveyor CI container/VM, which is having troubles downloading NW.js all of the time, too. I had added the Appveyor CI after your report #565, when I wasn't aware of the build issues on Windows after the Webpack upgrade recently. Appveyor is the only "sensible" Windows based CI service with Github integration. However, since I knew how bad it actually is, I avoided using it here until then. I'll have to figure out these build fails later today or tomorrow...

Regarding your pull request, I'd appreciate if there was someone else who could review this. I can't do this, obviously 😃.
I noticed that there are a couple of unfinished/missing translations which need to be improved/fixed. This should be done ideally before everything gets merged. I also noticed a little copy&paste error "Twtich" in settings.yml. Apart from that, it's looking good. If you have any questions regarding some translation strings and their meaning (which is sometimes hard to understand without context, I agree), just ask here or on Gitter. :)

@codecov
Copy link

codecov bot commented May 15, 2018

Codecov Report

Merging #567 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #567   +/-   ##
=======================================
  Coverage   84.03%   84.03%           
=======================================
  Files         472      472           
  Lines       12516    12516           
  Branches     1218     1218           
=======================================
  Hits        10518    10518           
  Misses       1903     1903           
  Partials       95       95

@dxball
Copy link
Contributor Author

dxball commented May 15, 2018

I've fix the 'Twtich' typo ☺️

@bastimeyer
Copy link
Member

@dxball There are a couple of missing translations left. Could you please add them, so I can merge this? Thank you!

@dxball
Copy link
Contributor Author

dxball commented Jun 8, 2018

@bastimeyer sorry.. I thought there will be someone can help the translation 😮

I've add the missing translations.

@bastimeyer
Copy link
Member

I'm having trouble finding a font that includes all of the used glyphs here. Streamlink Twitch GUI uses Roboto as primary font. Android does the same, but falls back to Noto Sans for CJK fonts.
I was trying to add Noto Sans from two of the available npm packages, but both of them seem to be limited to latin, cyrillic and greek letters. I don't want to let the app use the fonts installed on the user's system, because it's inconsistent. Same issue with other similar fonts, like Droid Sans. Why are there no npm packages for this? Am I missing something here?

@dxball
Copy link
Contributor Author

dxball commented Jun 10, 2018

I've no idea about the font issue 😅

@dxball dxball closed this Jun 10, 2018
@dxball dxball reopened this Jun 10, 2018
@bastimeyer bastimeyer merged commit 6e26997 into streamlink:master Jun 11, 2018
bastimeyer added a commit that referenced this pull request Jun 11, 2018
@bastimeyer
Copy link
Member

Thanks again for the translations! 👍

I've decided to not include other fonts. The Noto Sans CJK font family is way too large to embed it into the app and it's not worth the effort. However, the app is now properly referring to the Noto Sans system fonts, if installed on the user's system. This should be good enough.
2e5bc8c

@dxball dxball deleted the i18n/zh-tw branch July 29, 2018 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants