Skip to content

Conversation

@Viniciuss-Moreira
Copy link
Contributor

Gravacao.de.Tela.2025-10-15.as.04.04.37.mov

Resume

This PR refers to issue #448 to make the user experience easier and more dynamic, without having to reopen the app for the language change to take effect.

Changes

  • Add a new component (LanguageManager.swift) that locates strings and instantly deconstructs and constructs views as soon as the language is changed.
  • The “title” variable is now called “titleKey” in bottomsheetup.swift, applying to all viewmodels in the app. The purpose is to make it clear that this is localizable text that is going through a function, and not the final text displayed in the view. This facilitates future maintenance and understanding by new programmers in the codebase.
  • The text of the alert screen has been changed in the localizables to convey the idea of asking the user whether they want to keep the language change or not, rather than restarting the app.
  • Restructuring of views and viewmodels for language manager injection.

@Viniciuss-Moreira Viniciuss-Moreira force-pushed the feat/switch-language-no-restart branch from 28028da to b27e334 Compare October 15, 2025 18:52
@r1b2ns
Copy link
Contributor

r1b2ns commented Oct 19, 2025

Hi @Viniciuss-Moreira, I sent you some fixes you could take a look at.

Copy link
Contributor

@r1b2ns r1b2ns left a comment

Choose a reason for hiding this comment

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

Running the app, I realized that it’s working fine. These requested changes are just to keep the code cleaner.

@Viniciuss-Moreira Viniciuss-Moreira force-pushed the feat/switch-language-no-restart branch from 1206b4b to a1b9f84 Compare October 20, 2025 20:18
@Viniciuss-Moreira
Copy link
Contributor Author

Thanks @r1b2ns for the review!

@r1b2ns
Copy link
Contributor

r1b2ns commented Oct 24, 2025

I gonna review it this weekend

Copy link
Contributor

@r1b2ns r1b2ns left a comment

Choose a reason for hiding this comment

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

Very good job, man! In this PR, I just noticed some changes that aren’t related to your feature — for example, in LessonDetails. It would be great if you could implement the language override without changing those other parts.

Another thing is that you’re injecting the LanguageManager via @EnvironmentObject, but in some files, you’re still using the singleton. I think you can choose either approach, but not use both at the same time. Both are valid in my opinion — just not together.

Despite these comments, your code is very good. We’re just polishing it because this feature is very important and impacts the entire app.

@Viniciuss-Moreira Viniciuss-Moreira force-pushed the feat/switch-language-no-restart branch from a1b9f84 to a078eeb Compare October 28, 2025 03:34
@Viniciuss-Moreira Viniciuss-Moreira force-pushed the feat/switch-language-no-restart branch from a078eeb to a220785 Compare October 28, 2025 03:50
@Viniciuss-Moreira
Copy link
Contributor Author

Viniciuss-Moreira commented Oct 28, 2025

Thanks again man, for the review! the changes were very enlightening.

I returned to the previous LessonListModel structure but converted the static strings to dynamic strings with the new struct LocalizedLessonText, that observes the language manager and rebuilds the views.

I also returned to the original ForEach in LessonDetailView and only used the language manager injected into the views

If there’s anything else, I’ll be happy to see it.

@r1b2ns
Copy link
Contributor

r1b2ns commented Nov 2, 2025

ACK a220785

Copy link
Owner

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

Awesome work guys!

ACK a220785.

@thunderbiscuit thunderbiscuit merged commit a220785 into thunderbiscuit:master Nov 3, 2025
3 checks passed
@r1b2ns r1b2ns mentioned this pull request Nov 6, 2025
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