-
-
Notifications
You must be signed in to change notification settings - Fork 2
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 add-on #5
Conversation
* add English translation as template * add German translation
That is fine with me.
The strings in these two arrays are what need to be localized: unicodify/src/common/modules/data/Fonts.js Lines 6 to 21 in fa3fed0
It would be easy to dynamically convert those above IDs into Camel case and then use the
The variable is just those above IDs dynamically converted into Kebab case to use as an index into the |
Only the quotes replacement is done immediately. | ||
</span><br> | ||
<span class="helper-text" data-i18n="__MSG_optionHelperTextAutocorrectionEmoji__" data-opt-i18n-keep-children> | ||
For Emoji autocorrection, including <code data-i18n="__MSG_optionHelperTextAutocorrectionEmojiColonsCode__">:colon:</code> shortcodes and Emoticons, please also try out our <a href="https://addons.mozilla.org/firefox/addon/awesome-emoji-picker/" data-i18n="__MSG_optionHelperTextAutocorrectionEmojiLinkText__" data-i18n-href="__MSG_optionHelperTextAutocorrectionEmojiLink__">🤩 Awesome Emoji Picker</a> add-on. |
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.
While it does not effect this PR, note that the AMO URL will need to be updated after I create the Thunderbird and Chrome PRs for the Awesome Emoji Picker...
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.
Ah indeed, at best dynamically. Yeah, thanks for the notice.
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.
No problem.
That’s the way to go IMHO then.
Yeah, we cannot/should not o t6hsi with the translated strings of course. Never use translated strings as IDs… Your hint about nin-Unicode chars in many languages is indeed an important thing to keep in mind, I did not though about this. |
Just looked into it and I doubt it's so trivial. The problem is the Also, regarding i18n, we should never apply any effects on the localized strings. This should be done to allow translators to translate "superscript" e.g. in Russian (translated with DeepL) to this:
So they can still give an example on how it looks while using their own character set to properly translate the thing. BTW this line does nothing, at most it logs something to the console. |
Yes, of course. The IDs would stay the same. The translated strings would only be used for the title that is displayed to the user.
Yeah, I only wanted to have one copy of those strings to use for both the IDs and titles, to make this easier to maintain. I was thinking we could use some simple code like this to convert the strings into Camel case: function camelCase(atext) {
const [head, ...tail] = atext;
return head.toLowerCase() + tail.join('').split(" ").map(([h, ...t]) => h.toUpperCase() + t.join('')).join('');
}
// ...
const title = browser.i18n.getMessage(camelCase(id.toLowerCase())) || id; You could of course use some more complex code with regular expressions like this to also remove the hyphens and parentheses: function camelCase(atext) {
const [head, ...tail] = atext;
return head.toLowerCase() + tail.join('').replace(/(?<=\P{Alpha})\p{Alpha}\S*/gu, ([h, ...t]) => h.toUpperCase() + t.join('')).replace(/\P{Alpha}+/gu, '');
}
Some of the strings are "separator" to indicate that there should be a separator there, so
OK, I thought it would be nice for the users to be able see the effect of the options in their language without the translators having to manually convert anything. For example, if the Russian translator used
No, it converts the Font strings into an array symbols using Let me know if you need me to make any changes. |
Sorry but "simple"? The code you presented there is really long and one really needs to think about it to get what it does… 🙃
Well object.keys and then just filter.
I did not test, but actually should not make a difference. Array.from "creates a new, shallow-copied Array instance from an array-like or iterable object", i.e. it returns the new array. It does not modify the existing one. So you need to assign the result somewhere… |
Ah sorry, indeed it assigns thing in |
Oh, sorry. I edited my post and changed the code to make it more general after I wrote that. Here is the original simpler version, which assumes the string is lowercase. function camelCase(atext) {
const [head, ...tail] = atext.split(" ");
return head + tail.map(([h, ...t]) => h.toUpperCase() + t.join('')).join('');
}
Some of the strings in the |
Well… in any case it needs to be changed, because…
This assumption/aim is not there anymore if we localize. We should kjeep all strings in the |
function capitalizeEachWord(text) { | ||
// Regular expression Unicode property escapes and lookbehind assertions require Firefox/Thunderbird 78 | ||
// see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp#bcd:javascript.builtins.RegExp | ||
// return text.replace(/(?<=^|\P{Alpha})\p{Alpha}\S*/gu, ([h, ...t]) => h.toLocaleUpperCase() + t.join('')); |
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.
Why is this line commented out? Edit: Looks like you were trying to resolve the Parsing error: Invalid regular expression
with ESLint. Try updating the 2017
on this line: https://github.com/rugk/unicodify/blob/main/.eslintrc#L4 to at least 2019
or just 9
(see 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.
Ah yeah need to comment in again.
// convert string back to Symbol | ||
const caseId = caseByString[info.menuItemId]; | ||
const fontId = fontByString[info.menuItemId]; | ||
if (caseId) { |
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.
Maybe if (info.menuItemId in caseByString) {
to eliminate the temporary variables. Same below.
menuItemsToShow = contextMenuList.filter((id) => !fontValues.includes(id) ); | ||
} | ||
menuItemsToShow | ||
.filter((id, index) => (index !== 0 || id !== SEPARATOR)) |
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.
} else { | ||
menus.create({ | ||
"id": id.toString(), | ||
"title": browser.i18n.getMessage(menuTranslation[id]), |
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.
This line prevents the user from seeing the effect of the options. It will also fail if any of the strings have not been translated, as they currently have not for German.
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.
Yes, I do plan to move the effect of the translations into the translation itself. As said, to make things like надстрочный индекс (ˢᵘᵖᵉʳˢᶜʳⁱᵖᵗ)
possible and allow translators to customize the appearance, if they needs to.
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.
OK, as I said it would be possible either way since the changeFont()
function ignores non-ASCII characters...
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.
Okay okay, we'll try it like this.
@@ -5,146 +5,135 @@ import * as BrowserCommunication from "/common/modules/BrowserCommunication/Brow | |||
import { isMobile } from "./MobileHelper.js"; | |||
|
|||
import { COMMUNICATION_MESSAGE_TYPE } from "/common/modules/data/BrowserCommunicationTypes.js"; | |||
import { caseIds, fontIds, fonts } from "/common/modules/data/Fonts.js"; | |||
import { CASE, FONT, SEPARATOR, contextMenuList, menuTranslation, fontMap, caseByString, fontByString } from "/common/modules/data/Fonts.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.
Note that this will include at least four duplicate copies of the strings, which will both increase memory use and make this feature more difficult to maintain.
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.
Yeah, it exaggerated a little yesterday as I "only" wanted to use Symbols as IDs, but then stumbled upon all these errors that made it harder than it should be. E.g. you can only use strings in the contextMenu
API as an ID – respectively if you use a Symbol it does not match the Symbol that was passed to it, before.
Still need to think whether there may be a better solution.
src/common/modules/data/Fonts.js
Outdated
"fullwidth": "!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnopqrstuvwxyz{|}~", | ||
* @type {Object.<FONT, string>} | ||
*/ | ||
const fonts = { |
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.
This object should be frozen now.
Some status update, as this is blocking a lot and the last update was way too long ago: First of all, sorry for the delay. So I'm still not sure what to do… |
I am not sure why all the menu items are |
Did you test it with the latest commit in this branch/PR here? |
Yes, see here: There are still many problems as noted in my comments above, including that the user does not see the effect of the options (compare the above screenshot, with the screenshots here: rugk/awesome-emoji-picker#106 (comment)). I agree that many of the changes in this PR should be reverted, although feel free to do what you think is best. |
Superseded by #17 |
This first localizes the options page. I had one problem with a non-breaking space, but I could fix that by myself.
I suggest we review any change before pushing to the main branch, @tdulcet. Later I may also protect the main branch before we do any release to enforce this. (For now for small fixes that really need no review pushing to the
main
branch is okay, IMHO.)I guess you are a native speaker of English, so feel free to review the English parts.
(I doubt you can review the German parts, so you can ignore them, if you cannot. :) )
—
TODO:
UnicodeFontHandler
you seem to create the titles dynamically. This is of course clever and okay in general, however, obviously needs to be changed now. Also pay attention to how the keys in themessages.json
are currently named – you can build that dynamically, but yeah.What we'd need to decide is whether we write the
camelCaseStyle
e.g. in the translation file directly or not. At least for the casing stuff, I’d tend to write it in there, so translators could choose a different highlighting or so if that is required.(Also, while you are at it, expand the
aid
variable, it is way too short and not self-explaining.)