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

Localize default messages #846

Closed
wants to merge 15 commits into from
Closed

Conversation

miguelcast
Copy link

@miguelcast miguelcast commented Feb 16, 2018

Localize default messages

Add localize prop to Form component for translate messages to different languages. Fixes #1195

Checklist

  • I'm updating documentation
    • I've checked the rendering of the Markdown text I've added
    • If I'm adding a new section, I've updated the Table of Content
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
    • I've run npm run cs-format on my branch to conform my code to prettier coding style
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

@miguelcast miguelcast changed the title Bump version 1.1.0 Localize default messages version 1.1.0 Feb 16, 2018
README.md Outdated
@@ -1471,6 +1472,21 @@ One consequence of this is that if you have an empty string in your `enum` array

If you want to have the field set to a default value when empty you can provide a `ui:emptyValue` field in the `uiSchema` object.

## Localize default messages

You can change localize to default messages errors.
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can change localize to default messages errors.

=>

You can change the localization of the error messages.

README.md Outdated

Here are some examples from the [playground](http://mozilla-services.github.io/react-jsonschema-form/).

Here list of languages support [ajv-i18n](https://github.com/epoberezkin/ajv-i18n).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is the list of supported languages ajv-i18n.

README.md Outdated
```jsx
render((
<Form schema={schema}
localize="es" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

localization="es"

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "react-jsonschema-form",
"version": "1.0.2",
"version": "1.1.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

please don't change version here.

@edi9999
Copy link
Collaborator

edi9999 commented Apr 5, 2018

Can you add a test showing usage of this new feature ?

I would also like to think if we could add support for localization without adding a dependency to this repo (since it increases the bundle size for each user, despite not using every feature)

src/validate.js Outdated
) {
try {
ajv.validate(schema, formData);
localize[local] && localize[local](ajv.errors);
Copy link
Contributor

Choose a reason for hiding this comment

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

All avji18n will be imported. Why, insted of take a "string" in props, taking a function, that can perform the localization ?
The user (dev?) will can import only needed local function, and pass it.

Suggested change
localize[local] && localize[local](ajv.errors);
localize && localize(ajv.errors);

Copy link

Choose a reason for hiding this comment

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

I agree

Copy link
Member

Choose a reason for hiding this comment

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

@miguelcast do you see how it would work here? Then to use the form, we would use the following:

var localize = require('ajv-i18n/localize/ru');
...
<Form localize={localize} ... />

@miguelcast miguelcast changed the title Localize default messages version 1.1.0 Localize default messages Dec 11, 2018
@epicfaace
Copy link
Member

@miguelcast Can you please make the suggested changes?

Cognox S.A.S added 2 commits January 25, 2019 11:50
Copy link
Member

@epicfaace epicfaace left a comment

Choose a reason for hiding this comment

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

Thanks for your changes. A few more comments.

src/validate.js Outdated
) {
try {
ajv.validate(schema, formData);
localize[local] && localize[local](ajv.errors);
Copy link
Member

Choose a reason for hiding this comment

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

@miguelcast do you see how it would work here? Then to use the form, we would use the following:

var localize = require('ajv-i18n/localize/ru');
...
<Form localize={localize} ... />

test/validate_test.js Outdated Show resolved Hide resolved
Cognox S.A.S added 4 commits January 28, 2019 10:07
Function that receives the errors and you can modify them.
Remove ajv-i18n dependency.
const { definitions } = this.getRegistry();
const resolvedSchema = retrieveSchema(schema, definitions, formData);
return validateFormData(
formData,
resolvedSchema,
validate,
transformErrors
transformErrors,
localization
Copy link
Member

Choose a reason for hiding this comment

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

Why are you returning localization from validateFormData (i.e. what is the use case for doing so)?

@@ -294,5 +295,6 @@ if (process.env.NODE_ENV !== "production") {
transformErrors: PropTypes.func,
safeRenderCompletion: PropTypes.bool,
formContext: PropTypes.object,
localization: PropTypes.func,
Copy link
Member

Choose a reason for hiding this comment

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

Why did you rename it from localize to localization? Just want to hear your thinking on this.

@@ -127,7 +127,6 @@ function transformAjvErrors(errors = []) {
if (errors === null) {
return [];
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}
}

@@ -0,0 +1,53 @@
const messages = {
Copy link
Member

Choose a reason for hiding this comment

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

This is fine, but you might as well just import a localize function from ajv-i18n rather than writing your own convertMessagesText function for this playground example -- I'm guessing that's simpler? I'd assume that most of the use cases from localization would use an existing function corresponding to a language from ajv-i18n.

@epicfaace
Copy link
Member

@miguelcast do you have time to respond to my comments and make the requested changes on this?

@miguelcast
Copy link
Author

@miguelcast do you have time to respond to my comments and make the requested changes on this?

Excuse me for the delay, I thinking finish this feature these days.

@epicfaace
Copy link
Member

@miguelcast just bumping this!

@Maxhy
Copy link

Maxhy commented Jun 20, 2019

@miguelcast any chance to finish this or forward this stuff to someone else? A lot of localized features depends on it and as there is already a pending pull request, we are all waiting for it 😄 . Thanks!

@epicfaace
Copy link
Member

Anyone is welcome to pick this up if they would like this feature to be added.

@epicfaace
Copy link
Member

Closing this for now due to inactivity, though this PR could be a good starting point for a later implementation of localization.

@epicfaace epicfaace closed this Jun 2, 2020
@IgorLesnevskiy
Copy link

@epicfaace what's left to finish here? Guess I could do it.

@miguelcast
Copy link
Author

@epicfaace what's left to finish here? Guess I could do it.

The issue is long overdue, you have to update the PR and fix the unit tests to be able to finish it.

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.

trouble with native compatibility with ajv-i18n
7 participants