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

Add German Localization #68 #92

Merged
merged 22 commits into from
May 10, 2020
Merged

Add German Localization #68 #92

merged 22 commits into from
May 10, 2020

Conversation

TheVaan
Copy link
Collaborator

@TheVaan TheVaan commented May 9, 2020

Description

This PR is just to let you know there is something going on.

And to get comments: good way / bad way? ideas?

Pull requests with improvements or further translations (more languages) to my forks branch are welcome.

If you like the way I will continue building up the translation file for english and german and modifying the code where needed.

I don't know why there are 3 commits in this branch - all stuff is in
8392b7b
But it is way too late (or too early?) to think about git logic... 😴

Progress

Translate to German state:

  • Dashboard
  • Active Critters
  • Turnips
  • Villagers
  • Collection
  • User Lists
  • About
  • Settings
  • AC Helper+
  • HomeBar
  • Listing
  • Categories
  • ListItems
  • Shared

@Dimillian
Copy link
Owner

Wow this is a great effort! So one comment, every string that is used in a Text in the end (so text passed in a function that end up in a Text) don't need to be wrapped in NSLocalizedString you might want to double check some.

@TheVaan
Copy link
Collaborator Author

TheVaan commented May 9, 2020

Yeah I noticed that and already did corrected it last night but maybe I missed some. I‘m pissed that the localization file export doesn’t get the strings that are not in NSLocalizedString but in a text function.

@TheVaan TheVaan mentioned this pull request May 9, 2020
11 tasks
@ericlewis
Copy link
Collaborator

This is a good start, but ideally you should try switching as many string types to LocalizedStringKey, then you can drop most of the NSLocalizedString calls.

@TheVaan
Copy link
Collaborator Author

TheVaan commented May 9, 2020

I did some research (and testing): If you pass the text via text: "xyz" to a function where it will be filled into a text object with text(text) the localization doesn't work. Then you have to declare it like MyFunction(text: NSLocalizedString("xyz", comment: ""), ...) - I don't know why, but without it it will not be translated.

@ericlewis thank you, I'll take a look on LocalizedStringKey

@ericlewis
Copy link
Collaborator

ericlewis commented May 9, 2020

@TheVaan the reason is because any function passing a “string” to Text should actually have LocalizedStringKey as its type for the variable / arg passed, not String.

Basically:

// Bad, localization won’t work automatically
func test(str: String) -> some View {
Text(str);
}

// good, SwiftUI automagically looks up translation
func test(str: LocalizedStringKey) -> some View {
Text(str)
}

Edit: String & LocalizedStringKey aren’t exactly interchangeable, which is something important to keep in mind. If you need to compose Strings, you would still want to use NSLocalizedString. Also, if you do start changing call-sites to LocalizedStringKey you will likely need to change it elsewhere too, since strings aren’t interchangeable. This is good because it will make it obvious which things need translation as well.

@TheVaan
Copy link
Collaborator Author

TheVaan commented May 9, 2020

I updated my branch from master (what a disaster, so many changes) so I stumbled across the new todayDashboard and the new sections.
After rewriting SectionHeaderView to SectionHeaderView(text: LocalizedStringKey, ...) I saw that there is iteration over text which needs a hashable type so I reverted and now I'm using LocalizedStringKey directly inside the Text() call.

struct SectionHeaderView: View {
    let text: String
    var icon: String?
    
    init(text: String, icon: String? = nil) {
        self.text = text
        self.icon = icon
    }
    
    var body: some View {
        HStack(spacing: 6) {
            // ...
            
            Text(LocalizedStringKey(text))
                .font(.system(.subheadline, design: .rounded))
                .fontWeight(.bold)
                .foregroundColor(.acHeaderText)
        }
        // ...
    }
}

Yeay or neay?

Edit: this could have saved me so much time this night... now removing all NSLocaliationString's from SectionHeaderView....

@ericlewis
Copy link
Collaborator

@TheVaan if it is completely unavoidable then sure, but try to minimize any direct usage of LocalizedStringKey initializers as much as possible (similar to NSLocalizedString).

I can help you figure out how to remove them once you are done.

@ericlewis
Copy link
Collaborator

@TheVaan oh, another option to fix what you pointed out about Hashable would be to make LocalizedStringKey conform to Hashable. I’m not sure how possible that is, but it could be an option.

@TheVaan
Copy link
Collaborator Author

TheVaan commented May 9, 2020

@ericlewis yeah, I thought about extending LocalizedStringKey with Hash, but I seems to be not so easy for an amateur.

I think I didn't understand exactly how to reduce the direct usage of the initializer without messing the code up with extra lines of text. Or is this the right way and a better code style then inline text?

@ericlewis
Copy link
Collaborator

@TheVaan do you mean the code in the PR as it stands? If you’re done I am happy to review.

@TheVaan
Copy link
Collaborator Author

TheVaan commented May 9, 2020

@ericlewis There are missing many strings as it is a huge amount... copy and paste the strings isn't that quickly. Currently (full) translated: about, settings, ac helper+ info and the tabbar
I'll continue finding strings and merging them to Localizable.strings.

@ericlewis
Copy link
Collaborator

@TheVaan ping me when you’re ready for review, concerting this to a draft for now.

@ericlewis ericlewis marked this pull request as draft May 9, 2020 12:01
@ericlewis ericlewis changed the title WIP: Localization (draft) German Localization May 9, 2020
TheVaan added 3 commits May 9, 2020 15:36
- more text strings added to english localization
- translated Dashboard, Settings, AC Helper+, About to German
- modified a few helper classes for localization support (needs review!)
- changed DateFormatter to localizable date formats
- sorted entries of Subscribe-, About- and SettingsView
- removed obsolet entries
@TheVaan

This comment has been minimized.

@ericlewis
Copy link
Collaborator

@TheVaan I moved your TODO to the description so we can see the progress easier.

@Dimillian
Copy link
Owner

This is really awesome and will speed up localization of the UI for the other language (like French that I'll do) so much. Will look for the actual data a bit later :)

TheVaan added 2 commits May 9, 2020 23:25
- Tabbar: changed My Stuff to Collection
- CollectionView: changed name to "Collection" (same name as in tab bar)
- translated UserListView and CollectionView
@TheVaan
Copy link
Collaborator Author

TheVaan commented May 9, 2020

I have a problem in CollectionListView.swift line 93: because the selected tab name is rawValue the string will not be translated.

@Dimillian
Copy link
Owner

So for that you'll need to add the whole enum values as keys

enum Tabs: String, CaseIterable {
    case items, villagers, critters, lists
}

so in your localization file you'll have
"items" = "translation";

and

"When you'll stars some %@, they'll be displayed here" = "translation"

I think it should work.

@TheVaan
Copy link
Collaborator Author

TheVaan commented May 10, 2020

This is what I tried yesterday but it didn't work.
For now I'm using I quick but dirty fix, hopefully one of you guys has a better idea:

private var emptyView: some View {
    let tabName = NSLocalizedString(selectedTab.rawValue, comment: "")
    return Text("When you'll stars some \(tabName), they'll be displayed here.")
        .foregroundColor(.acSecondaryText)
}

TheVaan added 4 commits May 10, 2020 08:49
okay, only SearchView has text inside that needs to translate... thank you for already making it LocalizedStringKey <3
@Dimillian
Copy link
Owner

Quick comment: somehow you added back the whole views/dashboard group to your project. This should be completely gone. So you can safely remove dashboards folder from your Xcode and all source inside (not only references but files).

@TheVaan
Copy link
Collaborator Author

TheVaan commented May 10, 2020

Oh damn, I think that happened due to the merge conflict yesterday... folder will be removed in the next commit. Thank you!

Comment on lines -20 to +21
public func label() -> String {
switch self {
case .fish:
return "Fishes"
case .wallMounted:
return "Wall mounted"
case .nookmiles:
return "Nook Miles"
default:
return self.rawValue.capitalized
}
public func label() -> LocalizedStringKey {
return LocalizedStringKey(self.rawValue)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Replaced label() function because using rawValue as LocalizedStringKey makes this obsolet to transform value to readable label.

@TheVaan TheVaan marked this pull request as ready for review May 10, 2020 12:44
@TheVaan
Copy link
Collaborator Author

TheVaan commented May 10, 2020

@ericlewis @Dimillian Okay, I'm done. Have fun 😅

@Dimillian Dimillian changed the title German Localization Add German Localization #68 May 10, 2020
@Dimillian Dimillian merged commit 4bd97c3 into Dimillian:master May 10, 2020
@TheVaan TheVaan deleted the localization branch May 10, 2020 13:17
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.

3 participants