SwEnt-Group13 / Unio

The world’s largest campus life platform.
3 stars 1 forks source link

Feature/strings management #56

Closed Aurelien9Code closed 1 week ago

Aurelien9Code commented 1 week ago

This PR include strings and language management using standard usage

-> I willingly separated my commits in two main parts : file changes with Resource manager and without Because I am not quite sure if creating a Resource Manager is a good pratice. I saw it could be great to prevent passing context to a class containing no @Composable functions, but some forums said it could include memory leaks I will ask the assistants, but i put it here so that anyone can discuss it

-> The EventListOverview.kt and the Login.kt screens does not include these changes are they are in other pull requests and doing two times the changes would be stupid.

Thank you !

UPDATE : I will still ask the assistants for my Resource Manager, however i won't link it to the project because it causes big errors that I know how to fix, but I want to be sure that creating a Resource Manager is a good practice and is useful.

Aurelien9Code commented 1 week ago

Overall great changes! This builds strong foundations for the project and will enable scalability. However, I don't entirely grasp how one will be able to later change the selected language from inside the application, i.e. how can the ResourceManager look into values-fr instead of values ?

Thanks and good point ! Actually when keeping this structure : values/strings.xml for default language and values-.../strings.xml for any other language, the android system will switch automatically to the language of the user ; )

I wanted to do a test button in the settings to force language change (inside the app, not the whole OS) in the settings, but that will be in another commit haha

Aurelien9Code commented 1 week ago

see update in the PR message

oskar-codes commented 1 week ago

@Aurelien9Code I see, that's perfect! By the way, if you do not want this pull request to be merged yet, I'd recommend converting it to a draft pull request.

Aurelien9Code commented 1 week ago

@Aurelien9Code I see, that's perfect! By the way, if you do not want this pull request to be merged yet, I'd recommend converting it to a draft pull request.

Hey ! Sorry if i was not clear, but i do want to merge it haha that would allow me to make the changes on my other waiting branch

Also the question i had about the Resource Manager does not impact this merge as there is only the ResourceManager.kt file left, won't be an issue. If the assistants say it's a good idea, I will implement it in another PR. If not, I'll just delete the file. However, and as tomorrow it's Sunday so I'll probably won't have an answer soon, I would like to merge to the main; mainly because the most quickly it is merged the most quickly everyone doing his branch on the week can use it in the new code they try to implement

Aurelien9Code commented 1 week ago

To be more specific, the Resource Manager is not anymore linked to any file because of the big errors it created (which need half a day to resolve haha) so pulling the code to main won't cause problem now and the rest of the implementation of strings can still be used now ; )

Aurelien9Code commented 1 week ago

The changes look good! I would extract the hardcoded colors out of the EventType enum and add them to ui.theme.Color.kt in. order to have them all in one place

Ooooo good idea !

sonarcloud[bot] commented 1 week ago

Quality Gate Failed Quality Gate failed

Failed conditions
48.5% Coverage on New Code (required ≥ 80%)
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint