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 internationalization tooling #4233

Merged
merged 53 commits into from
Feb 28, 2020
Merged

add internationalization tooling #4233

merged 53 commits into from
Feb 28, 2020

Conversation

outofambit
Copy link
Contributor

@outofambit outofambit commented Jan 6, 2020

Resolves #3390

Changes:

Adds i18next, a js library for internationalization to the p5.js codebase. It lets us use translated for text appropriate to the user's browser/computer configuration. It doesn't translate text for us though, that must be done by hand. It provides language detection and translation loading/use.

I've set up i18next for use anywhere in the library, with translations files for English and Spanish to start. (I only filled out one Spanish translation right now though. 😢) I've integrated it with all the FES messages except the parameter validation ones, which would be a great next step.

I've also kept this out of the minified build (at least for now) since it affects the initial loading and size of the p5.js library. I think the impacts would be minimal, but I'm playing it safe.

Did a little cleanup in error_helpers.js along the way, too.

Screenshot

Screen Shot 2020-02-24 at 11 25 27 PM

PR Checklist

  • npm run lint passes
  • [Inline documentation] is included / updated
  • [Unit tests] are included
  • [Contributor docs] are included

@limzykenneth
Copy link
Member

limzykenneth commented Jan 6, 2020

Is the translation text included in the final build itself? I'm a bit worried that it would blow up the size of the library too much especially after including more and more translations over time.

An idea I had before is to distribute localized versions of the library depending on the browser's language setting that is used to download the library or serving different versions on the web editor. Not sure how viable it is though.

Another idea is to have the translations as an addon that we distribute along with the complete zip package and automatically include on the web editor that can be removed by the user if not needed.

@outofambit
Copy link
Contributor Author

Is the translation text included in the final build itself? I'm a bit worried that it would blow up the size of the library too much especially after including more and more translations over time.

@limzykenneth yep the translation text included in the final build, though only in the minified version since we disable FES in that build anyways. i'm going to see how many different strings there are for FES so we can get an idea of what the scale is here.

I also like your alternatives, especially the translations as an extra download add-on. (i agree automatically including them in the web editor would be ideal, too!) We could also host translations online so sketches can fetch them if needed. That would require the sketch to have internet access, but maybe that's okay?

@limzykenneth
Copy link
Member

We could also host translations online so sketches can fetch them if needed. That would require the sketch to have internet access, but maybe that's okay?

@outofambit As long as in the case where the sketch doesn't have internet access it fail gracefully it should be fine. We can distribute them through CDN (cdnjs or jsdelivr etc) so the whole build and release cycle can follow the library itself, while still offering ways to download them for use offline.

@lmccart
Copy link
Member

lmccart commented Feb 10, 2020

@outofambit checking in on this one. is this ready to merge or do we want to look into hosting the translations externally?

@outofambit
Copy link
Contributor Author

@lmccart i think the first version could be embedded and we can look into hosting externally aferwards! let me do a little more testing/documenting tonight to make sure this is good!

@outofambit
Copy link
Contributor Author

@lmccart i think this is ready for review + merge!

cc @limzykenneth in case he has time to look at this again!

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.

i18n for FES messages
3 participants