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

Add ajv-i18n support #1244

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Add ajv-i18n support #1244

wants to merge 2 commits into from

Conversation

askalione
Copy link

Hello! Add i18n for validation error messges using ajv-i18n.

@josdejong
Copy link
Owner

Thanks for your PR Alexey, this will be a nice improvement.

The PR itself is very hard to see what actually changed: about every line in textmode.js and treemode.js is changed. Can you please maintain the 2 space indentation, and if you would like to do any refactorings in the code do that in a separate PR?

@josdejong
Copy link
Owner

Please note that you can run npm run lint to detect linting issues, and use npm run lint -- --fix to fix as much as possible automatically.

@askalione
Copy link
Author

and use npm run lint -- --fix to fix as much as possible automatically.

Thanks!

I have fixed indents issues and now all checks have passed. Should i make new PR?

@josdejong
Copy link
Owner

Thanks for the update, the code looks good 👍 . No need to create a new PR, I'll squash it when merging.

I'll test your work functionally soon.

@josdejong
Copy link
Owner

I've done some testing, and it works like a charm, thanks @askalione!

Two feedbacks:

  1. Bundling the full ajv-i18n is quite large: the minified+zipped bundle goes from 231KB to 244KB. I'm wondering whether we can come up with a solution which doesn't require bundling of all the locales. The only idea I can come up with is to have the user pass the right translation file himself via the options, like:

    const options = {
      language: 'ru'
      ajv-i18n: require('ajv-i18n/localize/ru')
    }
    

    But that is not easy to use. Do you have any ideas in this regard? If not I guess we simply have to accept this for now.

  2. JSONEditor has a minimalist version of the bundle which basically strips the Ace and Ajv, and color picker libraries. Since those are quite large. The ajv-i18n library is also stripped, but now the dist/jsoneditor-minimalist.js throws an error when you open the editor with a JSONSchema. We can solve this by putting a try/catch around the require loading ajv-i18n, the same way as is done for Ajv itself in the file tryRequireAjv.js. Can you adjust that and make sure the code doesn't break when ajv-i18n is not available in the bundle?

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.

None yet

2 participants