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

Update i18n.md with information about Unicode isolation marks #1053

Closed
wants to merge 7 commits into from

Conversation

alyavasilyeva
Copy link
Contributor

Provide information about possible formatting issues when using placeables and how to solve them.

Context: grammyjs/i18n#47

Provide information about possible formatting issues and how to solve them.
Co-authored-by: KnorpelSenf <shtrog@gmail.com>
Copy link
Member

@KnorpelSenf KnorpelSenf left a comment

Choose a reason for hiding this comment

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

Thank you so much!

@KnorpelSenf KnorpelSenf requested a review from roj1512 May 15, 2024 21:30
@KnorpelSenf
Copy link
Member

@alyavasilyeva just some quick info about what's gonna happen next:

  1. @roj1512 (website maintainer) will do a final review
  2. Once he approves, you're done here and you don't have to do anything anymore
  3. Translators will take over and sync up your changes across all languages
  4. This will be merged

site/docs/plugins/i18n.md Outdated Show resolved Hide resolved
site/docs/plugins/i18n.md Outdated Show resolved Hide resolved
Co-authored-by: Roj <rojvv@icloud.com>
Copy link
Member

@roj1512 roj1512 left a comment

Choose a reason for hiding this comment

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

Thank you!

@roj1512 roj1512 added the ready for translation Translator intervention is required. label May 16, 2024
@niusia-ua
Copy link
Member

Don't we first merge third-party contributions to the main branch and then create PR with translations?

@KnorpelSenf
Copy link
Member

Don't we first merge third-party contributions to the main branch and then create PR with translations?

We used to do that so that you don't have to add a new remote, but I think we can stop doing that because GitHub advertises the changes in this pull request via a remote branch, doesn't it? So you can actually view the diff locally or even merge without cloning the fork. Can you try?

@niusia-ua
Copy link
Member

niusia-ua commented May 16, 2024

I don't know what you're talking about. If this is some new GitHub feature, please tell me where I can read about it.
As far as I know, I need to make a fork of a fork and make changes from my fork to the fork.

@KnorpelSenf
Copy link
Member

You never had to fork a fork, you always could just clone the fork and push to the branch. You automatically get write access to a branch of the third-party fork as soon as the branch is used to open a pull request.

However, you can actually just keep the main copy of this repository (no forks). You can then follow https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally?platform=linux&tool=cli#modifying-an-inactive-pull-request-locally to see the changes of a pull request. But it seems like those refs are read-only, so perhaps it is not that useful after all. We may want to keep on merging to a temporary branch and performing translations there.

@@ -617,6 +617,20 @@ bot.command("start", async (ctx) => {
});
```

::: warning Potential Formatting Issues
By default, Fluent uses Unicode isolation marks for interpolations.
Copy link
Contributor

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 I've heard this term. This link may help readers who are also new to the topic.

Suggested change
By default, Fluent uses Unicode isolation marks for interpolations.
By default, Fluent uses [Unicode isolation](https://github.com/projectfluent/fluent.js/wiki/Unicode-Isolation) marks for interpolations.

Copy link
Contributor

@quadratz quadratz left a comment

Choose a reason for hiding this comment

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

According to https://github.com/projectfluent/fluent.js/wiki/Unicode-Isolation#what-does-it-look-like

....
Assume the following Fluent message which receives a URL as an argument from the app:

# In English
privacy-more = Visit {$PRIVACY_URL} to learn more.
# In Arabic (courtesy of Google Translate)
privacy-more = تفضل بزيارة {$PRIVACY_URL} لمعرفة المزيد.

Let the URL end with a forward slash, like so:

{"PRIVACY_URL": "https://www.mozilla.org/privacy/"}

Without isolation, the rendered result shows the trailing slash on the wrong side of the URL:

تفضل بزيارة https://www.mozilla.org/privacy/ لمعرفة المزيد. ❌

With isolation, the forward slash is displayed correctly:

تفضل بزيارة ⁨https://www.mozilla.org/privacy/⁩ لمعرفة المزيد. ✅

....
If useIsolation is set to false, there's a risk that some translations won't look correctly in bidi scenarios. For RTL languages (Arabic, Hebrew, Persian and more). this happens when interpolated variables are LTR and start or end with weak characters (URLs, book or website titles, citations, Wi-Fi network names, addon names, etc.). For LTR languages, this happens when the interpolated variables are RTL and have leading or trailing weak characters (names and title of all sorts are the most common use-case here).
....

Based on the information above, are we certain it won't cause other issues if we turn off this feature?

@niusia-ua
Copy link
Member

You never had to fork a fork, you always could just clone the fork and push to the branch. You automatically get write access to a branch of the third-party fork as soon as the branch is used to open a pull request.

Wow, it really works! But it's quite inconvenient to make a clone of the website repository for each third-party contribution. Of course, third-party contributions are quite rare compared to our own, but it's still not convenient.

I think it would be better to accept third-party contributions in a temporary branch that translators can easily work with.

@niusia-ua niusia-ua requested a review from deptyped May 18, 2024 07:39
@KnorpelSenf
Copy link
Member

Should we merge this into a temporary branch for the remaining translations?

@niusia-ua
Copy link
Member

@KnorpelSenf I think so.

@niusia-ua niusia-ua added the 🇺🇦 UK Modifies or is related to the Ukrainian translations. label May 20, 2024
@KnorpelSenf
Copy link
Member

Thanks @alyavasilyeva! These changes will be merged via #1058 after translation is done.

@github-actions github-actions bot temporarily deployed to pull request May 21, 2024 15:06 Inactive
github-merge-queue bot pushed a commit that referenced this pull request Jun 1, 2024
… (#1058)

Co-authored-by: Aliya Vasilyeva <aliyavws@gmail.com>
Co-authored-by: Roj <rojvv@icloud.com>
Co-authored-by: Qz <quadratz@proton.me>
Co-authored-by: Nazar Antoniuk <antoniuk.nazar09@gmail.com>
Co-authored-by: Acer <>
Co-authored-by: E5F1AC <150493299+dzikrl@users.noreply.github.com>
Co-authored-by: Habemuscode <{ID}+{username}@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for translation Translator intervention is required. 🇺🇦 UK Modifies or is related to the Ukrainian translations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants