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

Mark more text for localization #2210

Closed
wants to merge 110 commits into from
Closed

Mark more text for localization #2210

wants to merge 110 commits into from

Conversation

quiple
Copy link
Contributor

@quiple quiple commented Dec 14, 2023

No description provided.

@quiple
Copy link
Contributor Author

quiple commented Dec 25, 2023

I think it should be fine to merge, what do you think?

@quiple
Copy link
Contributor Author

quiple commented Dec 27, 2023

@ansh

@lens0021
Copy link
Contributor

+1 from another Korean user. I've reviewed translations and confirming I do not find any typo or error.

@pfrazee pfrazee added the intl Internationalization label Jan 8, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you delete all the .po files just so we don't have a lot of merge conflcits??

@@ -91,7 +91,7 @@ export function Component({onSelect}: {onSelect: (url: string) => void}) {
<TextInput
testID="customServerTextInput"
style={[pal.borderDark, pal.text, styles.textInput]}
placeholder="e.g. https://bsky.app"
placeholder={_(msg`e.g. https://bsky.app`)}
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to localize a URL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I included it in the localization because of e.g.. Is it okay to wrap the URL in ${}?


type ReasonMap = Record<string, {title: string; description: string}>
const CommonReasons = {
[ComAtprotoModerationDefs.REASONRUDE]: {
title: 'Anti-Social Behavior',
Copy link
Contributor

Choose a reason for hiding this comment

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

@pfrazee Do we want to localize these strings?

const triggerPush = () => {
// TODO: implement local notification for testing
}
const triggerToast = () => {
Toast.show('The task has been completed')
Toast.show(_(msg`The task has been completed`))
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove these are no point in localizing the Debug.tsx file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like there are other localized strings already in the Debug.tsx file, does that make any difference?

Copy link
Contributor

@ansh ansh left a comment

Choose a reason for hiding this comment

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

Most of the changes from this PR were merged with #2440

So I'll leave it to @pfrazee if this needs to be merged.

But the best solution would be to create a new clean PR with the few leftover strings. Or at the very least delete the generated .po files from this PR so it can be reviewed in depth again.

I also left some comments about some text that we might not want to localize.

In any case, really appreciate your contributions @quiple . Most of them were merged in a different PR but thank you very much nonetheless!!

@quiple quiple marked this pull request as draft January 9, 2024 23:54
@quiple
Copy link
Contributor Author

quiple commented Jan 9, 2024

@ansh Thanks for review! I'll request a new PR later.

@quiple quiple closed this Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
intl Internationalization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants