Skip to content
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

Chore (i18n): Make punctuation usage consistent in translations #9610

Merged
merged 13 commits into from Jun 13, 2023

Conversation

sprocketc
Copy link
Collaborator

@sprocketc sprocketc commented Jun 8, 2023

Light refactoring related to translations (mostly), cherry picked from #8722

@github-actions github-actions bot added the :type/dev This label is used to indicate that an issue or PR is related to development tasks or changes that label Jun 8, 2023
Copy link
Collaborator

@logseq-cldwalker logseq-cldwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sprocketc Nice cleanup. Just the "..." to address before merging

@@ -1041,15 +1041,15 @@
[:div.r.flex.items-center.justify-between
[:div
(ui/tippy
{:html [:small (str (t :page/show-whiteboards) " ?")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? ? 😆

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to bring in the old comments. See #8722 (comment)
Punctuation should be part of the translation (also see #8722 (comment) that didn't make it into this PR). The question mark is superfluous in this case, so removing it is a better option in my opinion, instead of invalidating the translations to include it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with this change. Just thought the original was funny.

Punctuation should be part of the translation

Agree with '?' and '!' but what about delimiting characters like ':' or '-'? Moving those characters into translation will likely make it easier to forget to delimit in some languages. Grep for '(str (t' for examples. Whatever we decide we should document in the translation guide and if possible remove counter examples

Copy link
Collaborator Author

@sprocketc sprocketc Jun 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgetting to delimit could happen, but having the ability to properly translate the app should be our top priority in my opinion. RTL languages is also something that we need to take into account. I think @sawhney17 implemented a plugin, but I didn't have the chance to test it yet.

we should document in the translation guide and if possible remove counter examples

💯

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgetting to delimit could happen, but having the ability to properly translate the app should be the top priority in my opinion

Seems reasonable. Could you remove some of the counter examples from the above grep and document the punctation guidance?

Copy link
Collaborator Author

@sprocketc sprocketc Jun 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced all (str (t instances and invalidated the previous translations . We still have to fix some remaining cases (grep " (t ). When all of these are fixed, it might be a good idea to update our linter. I added a note to dev-practices.md for now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. 👍

@@ -215,7 +215,7 @@
#(state/set-assets-alias-enabled! (not alias-enabled?))
true)]
[:span
(restart-button alias-enabled-changed?)]]
(when alias-enabled-changed? (restart-button))]]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

([content] (loading content nil))
([content opts]
[:div.flex.flex-row.items-center.inline
[:span.icon.flex.items-center (svg/loader-fn opts)
(when-not (string/blank? content)
[:span.text.pl-2 content])]]))

(defn notify-graph-persist!
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point it could be nice if non-components fns moved to another ns as this ns seems like it's outgrowing its primary purpose of providing reusable components

src/main/frontend/components/file.cljs Show resolved Hide resolved
@sprocketc sprocketc added the :type/hold Hold this PR. won't merge for now label Jun 8, 2023
prefix (str (t :new-page) ": ")
chosen (if (string/starts-with? chosen prefix) ;; FIXME: What if a page named "New page: XXX"?
(string/replace-first chosen prefix "")
chosen)
Copy link
Collaborator Author

@sprocketc sprocketc Jun 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should fix various issues related to page autocompletion with names starting with "New page" or parts of it (depends on the active language). For example, typing [[New]] would highlight the wrong New on the autocomplete component. Creating a reference starting with New page: could also lead to unexpected behavior.

The "new page" label is now handled on the custom item renderer of page-search
135e322#diff-804cdbc15af06aba05e572b8dcee83da9a495773a899aa35f8ce42bbaa6afce0R157-R159
Note that this has a minor performance cost, because we are checking if the page exists, for each search result

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice that it fixes some issues but bummer that it adds another entity lookup call per item rendered. When I type within [[]] in lambda, I think the initial typing feels less responsive. If users notice then we can try to improve this

Copy link
Contributor

@tiensonqin tiensonqin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup!

@logseq-cldwalker logseq-cldwalker removed the :type/hold Hold this PR. won't merge for now label Jun 13, 2023
@logseq-cldwalker logseq-cldwalker changed the title Chore (i18n): Refactoring Chore (i18n): Make punctuation usage consistent in translations Jun 13, 2023
Copy link
Collaborator

@logseq-cldwalker logseq-cldwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sprocketc Thanks for making punctuation consistent across translations! 👍 🚢 Just have one question about deleting translations

@@ -77,7 +77,6 @@
:file-rn/or-select-actions-2 ". Estas acciones no estarán disponibles una vez cierres este panel."
:file-rn/legend "🟢 Acciones de cambio de nombre opcionales; 🟡 Cambio de nombre obligatorio para evitar el cambio de título; 🔴 Cambio destructor."
:file-rn/close-panel "Cerrar el panel"
:file-rn/all-action "¡Aplicar todas las acciones!"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were all these entries that had punctuation removed because you think some of them may choose different punctuation? Seemed like most of these would've just used the same ones e.g. append ({1}) for this one

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chatted with Konstantinos and these were removed since some languages may have the arguments in different positions

@@ -116,7 +116,7 @@
:file-rn/or-select-actions-2 ". These actions are not available once you close this panel."
:file-rn/legend "🟢 Optional rename actions; 🟡 Rename action required to avoid title change; 🔴 Breaking change."
:file-rn/close-panel "Close the Panel"
:file-rn/all-action "Apply all Actions!"
:file-rn/all-action "Apply all Actions! ({1})"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the first time we've used arguments so I've added something about this to the guide

@@ -1041,15 +1041,15 @@
[:div.r.flex.items-center.justify-between
[:div
(ui/tippy
{:html [:small (str (t :page/show-whiteboards) " ?")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. 👍

prefix (str (t :new-page) ": ")
chosen (if (string/starts-with? chosen prefix) ;; FIXME: What if a page named "New page: XXX"?
(string/replace-first chosen prefix "")
chosen)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice that it fixes some issues but bummer that it adds another entity lookup call per item rendered. When I type within [[]] in lambda, I think the initial typing feels less responsive. If users notice then we can try to improve this

@logseq-cldwalker logseq-cldwalker merged commit f051790 into master Jun 13, 2023
12 of 13 checks passed
@logseq-cldwalker logseq-cldwalker deleted the chore/i18n-refactoring branch June 13, 2023 16:55
@queeup
Copy link
Contributor

queeup commented Jun 16, 2023

@sprocketc, I just realize my language have many untranslated words. Then I checked it and found out you are adding just : or ´!´ most of the time and erase all strings on the other languages. Please do not do that. If its just a small change like them add it to the other language to instead erasing the sentences.

@sprocketc
Copy link
Collaborator Author

sprocketc commented Jun 16, 2023

@queeup I am sorry for the inconvenience. The point of this change was to avoid making assumptions about punctuation without further knowledge. I understand that in most languages simply adding the same punctuation would be correct, but that's not the case for every single language. The only way to be sure about this, was to make contributors like you reevaluate the translations. If you can guarantee that for a specific language, all cases of this PR can be reintroduced, I can fix this myself. We need a way (usually your help) to verify that.

@queeup
Copy link
Contributor

queeup commented Jun 16, 2023

You are right about most of the cases. Anyways don't worry about it. I just wanted to tell you about this. I will correct it and send a PR with new translations. I can check the other PR's and copy/past/correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:type/dev This label is used to indicate that an issue or PR is related to development tasks or changes that
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants