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

docs: Update Parameter for VueI18n Translation Function Parameter #2850

Merged
merged 2 commits into from
Aug 6, 2020
Merged

docs: Update Parameter for VueI18n Translation Function Parameter #2850

merged 2 commits into from
Aug 6, 2020

Conversation

jefrydco
Copy link

@jefrydco jefrydco commented Aug 4, 2020

🔎 Overview

Background

Since the structure of locale/<anything>.json is

{
  "code":  "en",
  "messages": {
    "required": "The {_field_} field is required"
  }
}

and on the example we instantiate VueI18n this way

const i18n = new VueI18n({
  locale: 'en',
  en: {
    validations: validationMessages
  }
});

I think the correct parameter for translation function is $t('validations.messages.required', values)

Issues affected

Nothing

## Background

Since the structure of `locale/<anything>.json` is

```json
{
  "code":  "en",
  "messages": {
    "required": "The {_field_} field is required"
  }
}
```

and instantiate VueI18n this way

```javascript
const i18n = new VueI18n({
  locale: 'en',
  en: {
    validations: validationMessages
  }
});
```

I think the correct parameter for translation function is `$t('validations.messages.required', values)`
@codecov
Copy link

codecov bot commented Aug 4, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2850   +/-   ##
=======================================
  Coverage   95.67%   95.67%           
=======================================
  Files          50       50           
  Lines        1319     1319           
  Branches      317      317           
=======================================
  Hits         1262     1262           
  Misses         57       57           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8096619...0560128. Read the comment docs.

@logaretm
Copy link
Owner

logaretm commented Aug 5, 2020

The example shows how to do so for any rule because the defaultMessage will get executed for every rule. Is the code in the example incorrect or produces bugs?

Based on https://kazupon.github.io/vue-i18n/guide/component.html#component-based-localization the structure of VueI18n constructor options parameter for declaring translations should be like this

```
{
  locale: 'en',
  messages: {
    en: {}
  }
}
```
@jefrydco
Copy link
Author

jefrydco commented Aug 6, 2020

The code in the example is incorrect. I've created simple reproduction for that here, VeeValidate Docs Correct Example. Please take a look on App.vue on line 50-57 and comment uncomment both of the configure function invocation for correct and incorrect result.

Using the following code (changes)

    configure({
      defaultMessage: (field, values) =>
        this.$t(`validations.messages.${values._rule_}`, values)
    });

The result will be correct

image

But using the following code (existing example)

    configure({
      defaultMessage: (field, values) =>
        this.$t(`validations.${values._rule_}`, values)
    });

The result will be incorrect

image


Updates

I also have updated the code example for VueI18n constructor parameter options. According to the following docs VueI18n: Component Based Localization the correct parameter options is:

{
  locale: 'en',
  messages: {
    en: {
      validations: validationMessages
    }
  }
}

not:

{
  locale: 'en',
  en: {
    validations: validationMessages
  }
}

The reproduction for this update also can be take a look on the VeeValidate Docs Correct Example above.

@logaretm logaretm merged commit f6d3b10 into logaretm:master Aug 6, 2020
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.

2 participants