-
Notifications
You must be signed in to change notification settings - Fork 324
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
make strings localizable #888
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for starting this @JessicaBarh
I left a few comment.
To quickly correct linter errors, you can run locally
jlpm run eslint
. And to check itjlpm run eslint:check
.
Thanks for your comments @fcollonval. I'm not to familiar with the test files so please let me know what I can modify if needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work @JessicaBarh Thanks a lot for it.
Regarding the tests, we can directly use the nullTranslator - as we are not testing the translator. I made suggestions in that sense.
@goanpeca I would appreciate a review from you on this one.
Looking into it now. Thanks a lot @JessicaBarh ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @JessicaBarh. Great work. there are some things that need to change like `-> ' and some places where it is better to combine into a full string as some languages might have different opinions on the ordering of sentences.
Finally translation will not work on variables
so we need to either apply trans to actual strings or handle that wherever those strings are generated.
Thanks a lot for working on this!
Thanks for pointing these things out @goanpeca!
What would be the best approach here to make |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this PR @JessicaBarh
@goanpeca thank you too to take the time for reviewing the PR
Hi @JessicaBarh this is merged now, but to answer your question: plural = number_of_files > 1 ? 's' : ''
label: 'Show file${plural}'
label: trans._n('Show file', 'Show files', number_of_files) Thanks! |
No description provided.