-
-
Notifications
You must be signed in to change notification settings - Fork 942
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
Introducing i18n URL patterns (#883) #1251
base: main
Are you sure you want to change the base?
Conversation
39ac0fb
to
322ba5e
Compare
…patterns are in use.
51b1304
to
01b487e
Compare
Hello 👋 This PR is a little bit of a beast, but I think that's unavoidable when bringing in translations. I think I've covered all HTML, Javascript and Python strings which look helpful to translate. Besides the current script for making the messages, the javascript can be done with |
|
||
<p>The committee will then review the incident and determine, to the best of their ability: | ||
<p>{% blocktranslate %}The committee will then review the incident and determine, to the best of their ability: | ||
<ul> | ||
<li>what happened</li> |
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.
These could be translated individually but would need context providing to explain the opening sentence.
I decided a translation block around the whole thing was easy, but it does make for a chunky piece with markup involved.
Hey @marksweb — wowser, yes. Indeed. 😅 Initial thoughts:
Do we need to rope in more help from the DSF members, and wider community? |
In terms of a goal, I think my goal with this is the facilitation of translation. With the way translations will fallback to the source language, it becomes a bit of a community task where we provide people with the tools to contribute the translations if they're using the site and would find it helpful/useful for it to be in their native language. In terms of creating the translations, there is a script which hints at some transifex integration. A tool like that is certainly the best way to gather translations. You're absolutely right that the URLs would change with this. Any path which doesn't have a language code in it, django will redirect to a language prefixed path. The locale middleware should select the most appropriate from those available. I've never had any issue with this coming into play, and I have left some paths outside of the i18n patterns. I split the download And in terms of flatpages, I think this comes down to how much content people want to translate. It may also become permissions vs knowledge based if flatpages need to be translated via admin opposed to a third party tool. I'm afraid I have never needed to translate a flatpage - that cms platform is where similar translations have occurred in my projects. |
I'm sorry but I'm organizing a Django Girls workshop this weekend I'll try to find a few time to made a review next week. But feel free to find other volunteers reviewers if you can, and also to merge if @carltongibson approve it. |
There's no rush here. We need to make sure we sure what the plan is. Measure twice... 🙂 |
Absolutely. This is one that can sit & wait until everyone's happy and maybe there's plans in place for sourcing the translations 👍 |
var copy_str = gettext("Press ⌘-C to copy"); | ||
var success = $('<span class="clipboard-success">').text(copy_str); |
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.
Minor nit: Indentation is off on line 37.
Also, while we are at it. I think we can use let
instead of var
.
Similarly elsewhere (for the lines at least that you're already touching in this PR).
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.
I can go back over and fix indentation, but hesitant to change var
for let
, event though it's so minor, because it's outside the scope of the PR.
@@ -11,8 +11,9 @@ define([ | |||
MobileMenuExport.prototype = { | |||
init: function(){ | |||
var self = this; | |||
var label = gettext('Menu'); |
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.
Indentation is off here + use let
/const
for variables in JS.
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.
I can go back over and fix indentation, but hesitant to change var
for let
, event though it's so minor, because it's outside the scope of the PR.
@@ -17,14 +17,15 @@ define([ | |||
'donation_id': donationId, | |||
'csrfmiddlewaretoken': csrfToken | |||
}; | |||
var success_copy = gettext('Card updated'); |
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.
Indentation is off here too.
<span class="page-current"> | ||
Page {{ page_obj.number }} of {{ page_obj.paginator.num_pages }} | ||
</span> | ||
<span class="page-current">{% blocktranslate trimmed with page_number=page_obj.number pages=page_obj.paginator.num_pages %} |
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.
Curious to know where this trimmed with ...
is coming from?
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.
@CuriousLearner trimmed
is an option to blocktranslate
which removes newline characters from the generated translations - just makes things a bit cleaner for translators in my experience. with
is how you then provide variables to the block.
Info & examples on this here; https://docs.djangoproject.com/en/4.1/topics/i18n/translation/#blocktranslate-template-tag
if lang_code and check_for_language(lang_code): | ||
if next_url: | ||
next_trans = translate_url(next_url, lang_code) | ||
if next_trans != next_url: | ||
response = HttpResponseRedirect(next_trans) | ||
|
||
activate(lang_code) | ||
response.set_cookie( | ||
settings.LANGUAGE_COOKIE_NAME, lang_code, | ||
max_age=settings.LANGUAGE_COOKIE_AGE, | ||
path=settings.LANGUAGE_COOKIE_PATH, | ||
domain=settings.LANGUAGE_COOKIE_DOMAIN, | ||
secure=settings.LANGUAGE_COOKIE_SECURE, | ||
httponly=settings.LANGUAGE_COOKIE_HTTPONLY, | ||
samesite=settings.LANGUAGE_COOKIE_SAMESITE, | ||
) |
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.
Great work here!
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.
Wow, took a lot of time to review! ☕
Great work indeed. I think we are on the right track, but a general direction of what all is needed is to be included.
I would suggest to have this work in terms of milestones. I think then we can focus on acquiring specific milestones and getting django apps to support translation.
This sets up i18n URL patterns with the language selector from the docs, beginning to address #883.
This also adds translation tags to the fundraising templates to being to address #377
As for the languages in settings, I've taken those which are currently in the locale directory.
Happy for any feedback on the current state of changes & also as to whether I should keep adding translation tags to the rest of the templates or do that as separate, smaller, PRs.