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

Feat(ajax responses): Improved localization handling, error reporting and success messages #363

Merged
merged 31 commits into from
Mar 5, 2020

Conversation

sr258
Copy link
Member

@sr258 sr258 commented Feb 17, 2020

The PR closes #44, closes #80 and closes #83. To standardize localization it introduces a dev dependency on i18next (very common npm package) and converts the custom replacement variables (%xyz, @xyz) used so far into more widespread {{xyz}} notation.

Implementations can use any localization library, but must add the property t(...) to the Express req object of the if using the Express adapter. They must also follow the {{...}} notation.

sr258 and others added 28 commits February 14, 2020 13:02
BREAKING CHANGE: ILibraryStorage.getFileStream is now async!
…elated issues where current implementation doesn't conform to the H5P standard
@sr258
Copy link
Member Author

sr258 commented Feb 27, 2020

The changes turned out to be huge, but now error handling is much improved and the ajax endpoint is more stable and nearly fully tested. This means we now have integration tests of nearly all AJAX endpoints (I didn't test installing libraries from the H5P Hub as we still can't mock downloads from there).

I had to change the interface of the library (breaking changes):

  • Removed TranslationService from constructor of H5PEditor (the translation service has been removed)
  • ILibraryStorage.getFileStream is now async (needed to check if files actually exist; if you don't do this, the library will throw the file system errors and stop execution in some cases)

The example now also adds i18next to the request object. I think it's good to stay localization library agnostic and leave it to the implementation what library to use. Do you agree, @JPSchellenberg ?

@JPSchellenberg you can review now.

@sr258 sr258 requested a review from JPSchellenberg February 27, 2020 18:49
@sr258 sr258 marked this pull request as ready for review February 27, 2020 18:49
@sr258 sr258 added this to the Milestone 1 milestone Feb 27, 2020
@sr258 sr258 added the [status] needs review PRs that are ready to be reviewed. label Feb 27, 2020
@sr258
Copy link
Member Author

sr258 commented Mar 3, 2020

I've merged the master

@sr258 sr258 merged commit 340945d into master Mar 5, 2020
@JPSchellenberg
Copy link
Member

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@sr258 sr258 deleted the feat/ajax-response branch March 8, 2020 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released [status] needs review PRs that are ready to be reviewed.
Projects
None yet
2 participants