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.

Sign up for GitHub

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

Jump to bottom

fix(lang): Add missing strings for Chinese (Simplified) and Chinese (Traditional) #6149

Merged
merged 1 commit into from
Aug 7, 2019
Merged

fix(lang): Add missing strings for Chinese (Simplified) and Chinese (Traditional) #6149

merged 1 commit into from
Aug 7, 2019

Conversation

misteroneill
Copy link
Member

@misteroneill misteroneill commented Jul 31, 2019 •
edited
Loading

Description

Add four missing strings for both Chinese variants we offer.

There are also script updates.

Requirements Checklist

  • Feature implemented / Bug fixed
  • Reviewed by Two Core Contributors

@misteroneill misteroneill mentioned this pull request Jul 31, 2019
Merged
2 tasks
brandonocasey
brandonocasey reviewed Aug 1, 2019
View reviewed changes
package.json Outdated
@@ -38,9 +38,11 @@
"postbuild:css:cdn": "postcss --verbose --config postcss.config.js -d dist/alt dist/alt/video-js-cdn.css",
"build:css:default": "sass --no-source-map src/css/vjs.scss dist/video-js.css",
"postbuild:css:default": "postcss --verbose --config postcss.config.js -d dist/ dist/video-js.css",
"prebuild:lang": "npm-run-all -s prebuild:lang:*",
"prebuild:lang:zh-rm": "shx rm -f lang/zh-Han*.json",
Copy link
Contributor

Choose a reason for hiding this comment

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

this should probably go in clean

brandonocasey
brandonocasey reviewed Aug 1, 2019
View reviewed changes
package.json Outdated
@@ -38,9 +38,11 @@
"postbuild:css:cdn": "postcss --verbose --config postcss.config.js -d dist/alt dist/alt/video-js-cdn.css",
"build:css:default": "sass --no-source-map src/css/vjs.scss dist/video-js.css",
"postbuild:css:default": "postcss --verbose --config postcss.config.js -d dist/ dist/video-js.css",
"prebuild:lang": "npm-run-all -s prebuild:lang:*",
"prebuild:lang:zh-rm": "shx rm -f lang/zh-Han*.json",
"prebuild:lang:zh-s": "shx cp lang/zh-CN.json lang/zh-Hans.json",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can stay as build:lang:chinese-s and build:lang:chinese-t

brandonocasey
brandonocasey reviewed Aug 1, 2019
View reviewed changes
package.json Outdated
@@ -181,7 +183,7 @@
},
"husky": {
"hooks": {
"pre-commit": "lint-staged"
"pre-commit": "npm run prebuild:lang && git add lang/ && lint-staged"
Copy link
Contributor

@brandonocasey brandonocasey Aug 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We should probably change lint-staged here rather than the husky hook. Should be a copy and past from
EDIT: link didn't work here is a new one

video.js/package.json

Lines 173 to 176 in 88d7c9c

"lang/*.json": [
"node build/translations.js",
"git add docs/translations-needed.md"
]

Copy link
Member Author

@misteroneill misteroneill Aug 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I tried but it didn't want to work. Will try again.

@brandonocasey brandonocasey self-requested a review August 1, 2019 15:05
@gkatsev
Copy link
Member

gkatsev commented Aug 1, 2019

FYI, we don't have a lang: prefix: https://github.com/videojs/conventional-changelog-videojs/blob/master/convention.md#type

gkatsev
gkatsev reviewed Aug 1, 2019
View reviewed changes
package.json Outdated
@@ -181,7 +183,7 @@
},
"husky": {
"hooks": {
"pre-commit": "lint-staged"
"pre-commit": "npm run prebuild:lang && git add lang/ && lint-staged"
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it's not! I thought I was adding the clones to the repo.

@misteroneill misteroneill changed the title lang: Add missing strings for Chinese (Simplified) and Chinese (Traditional) fix(lang): Add missing strings for Chinese (Simplified) and Chinese (Traditional) Aug 5, 2019
@misteroneill
Copy link
Member Author

Which makes more sense:

  1. Continue ignoring zh-Hans.json and zh-Hant.json and only using them as temporary build files.
  2. Add them to the repository, requiring more scripting to make sure they stay in sync and are added on commit.

My vote is 1.

@brandonocasey
Copy link
Contributor

brandonocasey commented Aug 5, 2019 •
edited
Loading

I prefer number 1 as well

@gkatsev gkatsev merged commit bd51e9e into master Aug 7, 2019
@gkatsev gkatsev deleted the lang/zh branch August 7, 2019 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brandonocasey brandonocasey brandonocasey approved these changes

@gkatsev gkatsev gkatsev approved these changes

Assignees
No one assigned
Labels
confirmed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@misteroneill @gkatsev @brandonocasey