Skip to content

Conversation

@philipgiuliani
Copy link
Contributor

@philipgiuliani philipgiuliani commented May 30, 2018

Sumary of proposed changes

This is my second approach to #878.
I have extended i18n to allow nested translations with the dot notation. So you can call i18n.t('qualityName.720', this.config). And with this change I added the ability to translate the badge and the quality name.

Result:
bildschirmfoto 2018-05-30 um 14 58 44

… another way to solve this issue would be to add a label to the <source> elements. That would be the same approach as captions is currently using. But this seems like a complicated change to me.

Copy link
Contributor

@friday friday left a comment

Choose a reason for hiding this comment

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

Smart! I like it. 👍

How about making the array reduce a method in utils.js like getIn or getDeep?
It could actually be used internally in Plyr to avoid code like x && x.y && x.y.z, since it's pretty much a simplified variant of lodash's get https://lodash.com/docs#get which is used for this purpose.

Regardless of the application of it, I would make sure it returns undefined (not {}) when it doesn't match though, and also use more readable variable names (like obj and key). Since i isn't an iterator, it's actually a misnomer.

Like I said last time, Sam's the maintainer and has the final word.

@friday friday changed the base branch from master to develop May 30, 2018 14:51
@philipgiuliani
Copy link
Contributor Author

Thanks for the feedback.

I will move it to the utils.js file and change the variable names of course.

It actually returns undefined when there is no match. Because object["unknownKey"] will return undefined. The || {} is to prevent it from crashing when the key does not exist.

@friday
Copy link
Contributor

friday commented May 30, 2018

I changed the base branch to develop (which is the branch for submitting PR's), but since you branched off from master it contains some of Sam's commits now. You should be able to fix this by rebasing from develop git rebase develop and pushing with force or "force with lease."

@philipgiuliani
Copy link
Contributor Author

Thank's for the review. For some reason the ESLint tweak commit remained in my branch, but I guess this is not a problem.

@friday
Copy link
Contributor

friday commented May 30, 2018

It actually returns undefined when there is no match. Because object["unknownKey"] will return undefined. The || {} is to prevent it from crashing when the key does not exist.

That's the problem though. And very easy to fix:

function getDeep1(path, object) {
    return path.split('.').reduce((obj, key) => obj[key] || {}, object);
}

function getDeep2(path, object) {
    return path.split('.').reduce((obj, key) => (obj && obj[key]), object);
}

console.log('Test 1', getDeep1('a.b.c', {a: {b: {c: 'd'}}})); // 'd'
console.log('Test 2', getDeep2('a.b.c', {a: {b: {c: 'd'}}})); // also 'd'
console.log('Test 3', getDeep1('a.b.c', {})); // {}
console.log('Test 4', getDeep2('a.b.c', {})); // undefined

@friday
Copy link
Contributor

friday commented May 30, 2018

You can remove commits, combine etc with an interactive rebase (ex git rebase -i HEAD~9), but do read up on the topic if you haven't used this before. I'm not sure it's a problem per se. It depends on Sam's merging strategy.

// Get a nested value in an object
getDeep(object, path) {
return path.split('.').reduce((obj, key) => (obj && obj[key]) || undefined, object);
return path.split('.').reduce((obj, key) => obj && obj[key], object);
Copy link
Contributor

Choose a reason for hiding this comment

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

Haha.. my bad! I caught this and edited right after posting it, but you were faster.

@philipgiuliani
Copy link
Contributor Author

@sampotts What do you think about this changes?

Copy link
Owner

@sampotts sampotts left a comment

Choose a reason for hiding this comment

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

It looks good. Just a very minor change to please my OCD! 😂

case 'quality':
if (utils.is.number(value)) {
return `${value}p`;
const qualityName = i18n.get(`qualityName.${value}`, this.config);
Copy link
Owner

Choose a reason for hiding this comment

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

Small one but could we call the variable label and maybe the property for i18n as qualityLabel?

@philipgiuliani
Copy link
Contributor Author

Sure! I've renamed the variable 😄

@sampotts sampotts merged commit 5a445ae into sampotts:develop May 31, 2018
@sampotts
Copy link
Owner

Thanks mate! Awesome solution btw. I'll deploy a new version shortly. I just need to work on a small UI bug I've noticed.

@philipgiuliani philipgiuliani deleted the translate-qualities branch May 31, 2018 14:02
filips123 pushed a commit to filips123/plyr that referenced this pull request Nov 22, 2018
Translate quality badges and quality names
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