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 literals, which are not yet localized. #295

Merged
merged 2 commits into from
Apr 2, 2019
Merged

Localize literals, which are not yet localized. #295

merged 2 commits into from
Apr 2, 2019

Conversation

rumata28
Copy link

@rumata28 rumata28 commented Mar 14, 2019

  • use "translate" where it is available
  • Script.js changed to accept "translate" as an input parameter of exported function
    • Changed 3 places, which used the "Script.js", so that the "translate" module passed as a parameter down to function as a parameter value
  • Listener.js - refactored, so that translation would be obtained on rendering, not on module loading. Language switching would work correctly that way.

Closes #297

@rumata28
Copy link
Author

I also changed in some places 'Must provide a value.' to translate('Must provide a value') + '.'.
The rationale of that is to have just one literal to localize: Must provide a value instead of 2 almost equal literals: Must provide a value and Must provide a value..
In most of other places in the project you have Must provide a value - without dot at the end.

@barmac
Copy link
Member

barmac commented Mar 15, 2019

First and foremost thank you for your pull request! :) I left a few comments in the code. Please make sure that you adhere to our commit guidlines, which are linked in the CONTRIBUTING.md file.

@rumata28
Copy link
Author

Thank you for review of this PR.

I made a new commit with changes, which address (I hope) your remarks.

@pinussilvestrus
Copy link
Contributor

pinussilvestrus commented Mar 19, 2019

Hi @rumata28,
it would be very cool if you can squash your commits and clean up the commit history with the style @barmac posted above. As a result, we should have fewer commits which make it clear, what the pull request is supposed to add or to fix. If you need advice, don't be afraid to ask 🙂

@rumata28
Copy link
Author

Squashed and force-pushed it back to the branch. Hope I did not screw it up. :)

@pinussilvestrus
Copy link
Contributor

Thanks! Just one more thing: Can you modify your commit message to fit our guideline? It would be something like

feat(providers): improve overall localization

You could even consider splitting the commit into 2 or 3 since (as you described above, cf. #295 (comment)) there are different things achieved inside your pull request.

However, @barmac can you have a look at whether the made changes fit your requests?

@rumata28
Copy link
Author

Changed commit message as per recommendation of Niklas Kiefer.
This is still one commit, all changes are dedicated to improving overall localization.

If you see that it is not ok, could you please be more specific how exactly you would like to split it into several?

@barmac
Copy link
Member

barmac commented Mar 20, 2019

I've added a few more comments. Let's fix this, as we're close to merge :)

@rumata28
Copy link
Author

Resolved both findings.

Also, found some 2 more options list - localized them too (those are execution and task listener event types).

@rumata28
Copy link
Author

I added one more commit dedicated to translation of element template.

Localized list of templates, labels of template fields, validation messages.
Copy link
Member

@barmac barmac left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you for your contribution! 🚀

@merge-me merge-me bot merged commit 37dc92e into bpmn-io:master Apr 2, 2019
@ghost ghost removed the needs review Review pending label Apr 2, 2019
@rumata28 rumata28 deleted the fix-localization branch April 22, 2019 12:47
@rumata28
Copy link
Author

Guys, do you have any idea on when (or if) you plan to publish it?

@barmac
Copy link
Member

barmac commented Apr 24, 2019

Sure, it's now published in v0.30.0.

@rumata28
Copy link
Author

Thank you!

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.

I found a bug of i18n in bpmn-js-properties-panel
4 participants