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

Possibility to set default region #321

Merged
merged 3 commits into from
Feb 15, 2022

Conversation

paetz
Copy link
Collaborator

@paetz paetz commented Feb 10, 2022

Resolves #248

@paetz paetz requested a review from chrgernoe February 10, 2022 20:49
@chrgernoe chrgernoe changed the title possibility to set default region Possibility to set default region Feb 14, 2022
Copy link
Member

@chrgernoe chrgernoe left a comment

Choose a reason for hiding this comment

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

This feature shows me that we have to explain carefully inside the next release notes what has been changed otherwise the user will only accidentally find this feature.

@@ -78,8 +86,18 @@ class RegionManagerActivity : BaseNavigationActivity() {
currentCountryName = region.country.orEmpty()
}

var regionDispName = region.name.orEmpty()
var bgColor = Color.WHITE
Copy link
Member

Choose a reason for hiding this comment

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

As already mentioned, hard-coding colors makes re-styling complicated. I would define a color with a specific name. Since there are a lot of such lines in the code, it should be done in a separate PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, of course. This should be R.color.colorSecondaryLight. Strange that I missed that.

@@ -159,4 +177,21 @@ class RegionManagerActivity : BaseNavigationActivity() {
_updateHandler.finish()
}
}

private fun _selectDefaultRegion(regionId: Int) {
val key = getString(R.string.default_region)
Copy link
Member

Choose a reason for hiding this comment

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

Why is the German word Standardgebiet used internal as settings key? Because this string is defined in a file which is used to translate the app into other languages, this key changes for each language. It makes no sense and even can lead to the fact that the settings get lost, if the language is changed. I know, at the moment this cannot happen because the app is only available in German but in general it should be avoided. Why not just defining it in the setup.xml?

@paetz
Copy link
Collaborator Author

paetz commented Feb 15, 2022

This feature shows me that we have to explain carefully inside the next release notes what has been changed otherwise the user will only accidentally find this feature.

Just like the pinning/archiving feature of Threema which I had not known before you told me. However, I have the same opinion and would prefer more visibility without adding additional UI widgets like a checkbox.

@paetz paetz requested a review from chrgernoe February 15, 2022 21:54
chrgernoe
chrgernoe previously approved these changes Feb 15, 2022
@@ -160,6 +163,8 @@
<string name="only_official_summits">Nur offizielle Gipfel</string>
<string name="only_official_routes">Nur offizielle Wege</string>

<string name="default_region">Standardgebiet</string>
Copy link
Member

Choose a reason for hiding this comment

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

I think this definition is not used anymore after your last change.

@paetz paetz merged commit 84cbb15 into master Feb 15, 2022
@paetz paetz deleted the feature/248_possibility_to_set_a_default_region branch February 21, 2022 08:14
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.

As a user, I want to be able to set a region or country as default
2 participants