Rewrite rosetta.js #295
Rewrite rosetta.js #295
Conversation
* drop jQuery * fix various js bugs * add some new improvements
|
Ah thank you, I've long wanted to get rid of that ugly jQuery code! cheers! |
|
Great, I was hoping you wouldn't want to keep jQuery either :) Just added a small fix for an error when reflang is not used. And with optional chaining, which was a later addition but has the same browser support as all the other stuff. I'll squash the commits at the end. |
| const everyOrigVarUsed = origVars.every((origVar) => transVars.includes(origVar)); | ||
| const onlyValidVarsUsed = transVars.every((transVar) => origVars.includes(transVar)); | ||
| const valid = everyOrigVarUsed && onlyValidVarsUsed; | ||
| this.previousElementSibling.classList.toggle("hidden", valid); | ||
| }); |
There was a problem hiding this comment.
In the testproject there's a string with curly braces: %{count} observations. I thought gettext works only with the %(count)d syntax. I'm not sure how this is supposed to be handled exactly.
There was a problem hiding this comment.
Yes that looks wrong 🤔
This is correct thought:
_("{observations:d} observations").format(observations=10)which will produce
#, python-brace-format
msgid "{observations:d} observations"
msgstr ""
There was a problem hiding this comment.
Ah, I didn't realise that's allowed too, I guess there's no reason why it wouldn't work. But the current regex doesn't recognise {observations:d} as a variable. We could change it to \{[^\s}]*\}, which would follow what the regex does for the percent syntax.
But are the translators always supposed to use the exact same modifier, or is there perhaps a use case to have something different in another language?
(Btw, compilemessages won't warn about missing variables with the curly braces. It seems to do that only for the percent syntax. I guess this is why all the django docs examples are written that way.)
There was a problem hiding this comment.
I've changed the regex here: 3fdca87
This expects the exact same str.format modifiers to be used in translations.
| // Focus on the first textarea on page load | ||
| if (i === 0) { | ||
| textarea.focus(); | ||
| } |
There was a problem hiding this comment.
Thinking about accessibility, I guess focusing on the first textarea is probably not a good idea:
- I imagine people who use the tab key for navigation expect it to start from the beginning of the page.
- Making an on-screen keyboard show up on page load is likely not helpful in most cases either.
- Some people use the space bar to scroll down, which makes a change in the translation instead.
So I'd rather remove this.
There was a problem hiding this comment.
Removed the focus on page load here: b63db38
|
Sorry for the delay on this PR. I'm also working on #290 at the same time, and they are somewhat related (the ajax calls to handle the automatic translations and we've found some issues in the regexps that parse the string formatting variables). Not yet sure how to best reconcile these two PRs. |
|
No worries, I didn't mean to rush you. If that PR gets merged first then I can sort out the conflict easily. I've already added the global flag to that regex here too. If there will be more js changes on the other PR that shouldn't be much more difficult either. |
|
Hello @balazs-endresz I just merged #290 and it seems your PR is still mergeable. Do you think it's ready for a final review and merging, or are you still working on it? Thanks! |
|
It's ready for review. And once you've tested it I can squash the commits before merging. Or I can do that now with a rebase if you prefer. |
|
Merged via 03d9f38. Thank you so much for your work on this! |
All Submissions:
pip install tox && tox)I don't think this needs any test changes and doesn't add any new options to toggle either.
I've ended up rewriting
rosetta.jswithout jQuery, which is actually very easy now.The django admin uses a lot of modern js too, so I don't think there are any browser compatibility concerns nowadays.
Not having to deal with jQuery upgrades should be a positive too.
It fixes several js related issues and adds a few improvements:
attrandpropwhen toggling checkboxes)<code>tags (some of these were not stripped when using the server side translation)