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 hebrew locale #2573

Merged
merged 1 commit into from
Jun 4, 2024
Merged

Update hebrew locale #2573

merged 1 commit into from
Jun 4, 2024

Conversation

ShlomoCode
Copy link
Contributor

@ShlomoCode ShlomoCode commented May 30, 2024

I am a native speaker

Fixes # (issue)

Misc Checklist

  • My change requires a documentation update on Sparkle's website repository
  • My change requires changes to generate_appcast, generate_keys, or sign_update

Testing

I tested and verified my change by using one or multiple of these methods:

  • Sparkle Test App
  • Unit Tests
  • My own app
  • Other (please specify)

(Describe all the cases that were tested)

macOS version tested: 14.5

@zorgiepoo
Copy link
Member

zorgiepoo commented May 31, 2024

GitHub won't allow me to merge this because it is still a draft.

@ShlomoCode
Copy link
Contributor Author

Yes, I made a few quick fixes, in the next few days I'll finish it and get it out of draft mode

@ShlomoCode ShlomoCode marked this pull request as ready for review June 2, 2024 00:47
@ShlomoCode
Copy link
Contributor Author

@zorgiepoo It's ready to merge

"Ready to Install" = "מוכן להתקנה";

/* No comment provided by engineer. */
"Should %1$@ automatically check for updates? You can always check for updates manually from the %1$@ menu." = "האם %1$@ צריך לבדוק באופן אוטומטי אם יש עדכונים? אתה תמיד יכול לחפש עדכונים באופן ידני מהתפריט של %1$@.";
Copy link
Member

Choose a reason for hiding this comment

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

The first %1$ should be %1$@ I think.

Copy link
Member

Choose a reason for hiding this comment

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

Also the second one might be wrong too.

Copy link
Contributor Author

@ShlomoCode ShlomoCode Jun 2, 2024

Choose a reason for hiding this comment

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

It seems to me that it is wrong only in GitHub/VSCode's display which is LTR, and therefore causes a distorted display of mixed Hebrew (RTL) and English (LTR) text
When I copy the line to a div that is set to RTL, it displays properly: https://jsitor.com/WNpswH34x
CleanShot 2024-06-03 at 02 23 08@2x

Copy link
Member

Choose a reason for hiding this comment

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

Screenshot 2024-06-02 at 9 07 03 PM

Is this how you expect it to look like? It's best to check how the strings look at runtime in an app. I reset the defaults of the Sparkle Test App with defaults delete org.sparkle-project.SparkleTestApp and launched the app twice to get the permission prompt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it looks great
I have now also tested the all flow with Sparkle Test App and the format is valid, there are some minor RTL issues but I will file a separate issue on that

"Updating %@" = "מעדכן את %@";

/* No comment provided by engineer. */
"Use Finder to copy %1$@ to the Applications folder, relaunch it from there, and try again." = "השתמש ב-Finder כדי להעתיק את %1$@ לתיקיית היישומים, הפעל אותו מחדש משם ונסה שוב.";
Copy link
Member

Choose a reason for hiding this comment

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

Formatter token looks like it may be wrong here too.

Copy link
Contributor Author

@ShlomoCode ShlomoCode Jun 2, 2024

Choose a reason for hiding this comment

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

Same as #2573 (comment)

@zorgiepoo
Copy link
Member

The Hebrew localization is not currently enabled in SUUpdatePermissionPrompt.xib. It can be enabled in Xcode's File inspector for that file.

@ShlomoCode
Copy link
Contributor Author

ShlomoCode commented Jun 2, 2024

I have activated the SUUpdatePermissionPrompt file now

@zorgiepoo
Copy link
Member

zorgiepoo commented Jun 4, 2024

Thank you. I'll merge after the CI automation is done verifying the changes.

(One note: it's easier for me to review/track changes if you don't force push commits on the pull request branch. Also typically people create a separate branch when merging changes so they can keep their 2.x branch in sync. Edit: never mind, I see GitHub provides me of a way of comparing different revisions still. Oh well).

@ShlomoCode
Copy link
Contributor Author

Would you like to wait for me to add RLM tags or should we do it in a new PR?

@zorgiepoo
Copy link
Member

Let's separate it and wait until this one merges.

@zorgiepoo zorgiepoo merged commit 3c180ef into sparkle-project:2.x Jun 4, 2024
2 checks passed
@zorgiepoo zorgiepoo added this to the 2.7 milestone Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants