-
-
Notifications
You must be signed in to change notification settings - Fork 51
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: i18n for card text and French translation #39
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This looks like a great solution 👍 For the Vercel deployment and automated tests to work, the dependencies should also be added to the At some point (probably in another PR) it may be a good idea to try consolidating these to avoid repetition. Vercel doesn't work with poetry too well from what I've seen. |
The way it is isn't bad, but it may nice to take advantage of pluralization groupings in the i18n lib. If we use keys for I also think it's fine to get rid of the "just now" case from the relative time function to simplify things and just go straight to "x seconds ago" instead. eg. en-us:
views: "%{number} views"
seconds-ago:
one: "1 second ago"
many: "%{count} seconds ago"
minutes-ago:
one: "1 minute ago"
many: "%{count} minutes ago"
hours-ago:
one: "1 hour ago"
many: "%{count} hours ago"
days-ago:
one: "1 day ago"
many: "%{count} days ago"
months-ago:
one: "1 month ago"
many: "%{count} months ago"
years-ago:
one: "1 year ago"
many: "%{count} years ago" fr:
views: "%{number} vues"
seconds-ago:
one: "il y a 1 seconde"
many: "il y a %{count} secondes"
minutes-ago:
one: "il y a 1 minute"
many: "il y a %{count} minutes"
hours-ago:
one: "il y a 1 heure"
many: "il y a %{count} heures"
days-ago:
one: "il y a 1 jour"
many: "il y a %{count} jours"
months-ago:
one: "il y a 1 mois"
many: "il y a %{count} mois"
years-ago:
one: "il y a 1 an"
many: "il y a %{count} ans" |
That should make things neater 👍 |
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.
Looks great, thanks for the progress. Just a few more things to look into.
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.
A few small things
I added some test cases, with the fixes suggested above, they should pass. (Btw, in case you didn't know, suggestions can be committed in the GitHub UI from the "Commit suggestion" dropdown button. Just letting you know since it looks like you've been committing all the changes manually.) |
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.
Looks great! Thanks for the contribution 🎉
Anything else you want to add before I merge?
I added some steps to contribute. They would probably need to be refined a bit but should be good for now. |
Co-authored-by: Jonah Lawrence <[email protected]>
Co-authored-by: Jonah Lawrence <[email protected]>
Co-authored-by: Jonah Lawrence <[email protected]>
Co-authored-by: Jonah Lawrence <[email protected]>
d1e96af
to
f2512b4
Compare
Thanks again for the contribution! |
Summary
This PR adds the python-i18n module to handle localisations for the users and an optional parameter
lang
to the API query parameters, which can be used to set a user's locale. This PR only adds translations for en-us and fr for now. For the rest, should they be crowd-sourced via a mono-issue worked on by multiple users, or should they be included within the scope of this PR?Type of change
How Has This Been Tested?