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

Update Dutch language file #3297

Closed

Conversation

nickygerritsen
Copy link
Contributor

Description

While working on #3296 I saw the Dutch language file was outdated and contained a wrong translation. I updated it to have the new values.

Specific Changes proposed

This adds missing translations that were present in the English file and updates the translation of captions-related strings.

Requirements Checklist

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

@nickygerritsen nickygerritsen force-pushed the update-dutch-language branch from 3b4abc5 to f575dc5 Compare May 4, 2016 07:55
"subtitles off": "Ondertiteling uit",
"Captions": "Ondertiteling",
"captions off": "Ondertiteling uit",
"subtitles off": "ondertiteling uit",
Copy link
Contributor

Choose a reason for hiding this comment

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

Capitalization ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The source texts aren't capitalized either, so I didn't do that for the Dutch ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do wonder why the English versions are not capitalized, as IMHO it looks strange in the track menu's.
@videojs/core-committers any idea why this is the case?

Copy link
Contributor

Choose a reason for hiding this comment

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

And even if it is intentional, I think that in that particular spot, it would be a stylistic choice, for which there is the text-transform css property.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's because the source text is the track's kind + ' off' in https://github.com/videojs/video.js/blob/master/src/js/control-bar/text-track-controls/off-text-track-menu-item.js#L23

I don't see a reason to not capitalise the translation if that's preferred. I'd rather capitalise the English too, but that would also require including the en translations which doesn't happen now (en.json is there as a template).

Copy link
Contributor

Choose a reason for hiding this comment

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

label = options['kind'].charAt(0).toUpperCase() + options['kind'].slice(1); ?

Copy link
Member

Choose a reason for hiding this comment

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

I'm chalking it up to a stylistic choice unless everyone except @mmcc prefers the capitalization :P

Copy link
Member

Choose a reason for hiding this comment

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

Also, the menu has a text-transform css applied to it in the default skin, so, it would all be lower case. Though, that's also easily solved via a text transform. Ultimately, it doesn't matter that much what the translation files say specifically, as long as the keys match what's in the code, a user can set whatever they want via css.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what's the conclusion? With or without capitalization?

Copy link
Member

Choose a reason for hiding this comment

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

I would just leave it lowercase.

@gkatsev
Copy link
Member

gkatsev commented May 11, 2016

LGTM

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

Successfully merging this pull request may close these issues.

4 participants