-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Allow sensor expiry notification setting to be translated #2677
Allow sensor expiry notification setting to be translated #2677
Conversation
This doesn't preserve the existing text strings. |
@jamorham I agree that the title of my PR only claims that I am making the title and summary translatable. But, in reality, I have also made slight modifications. TitleThe title was: But, it's not an alert. it is a notification, isn't it? Why didn't I call it Sensor expiry notification? We allow user to customize a notification. They can choose a silent sound for an alert. Then, it becomes a notification. So, the user can make any alert become just a notification. SummaryThe summary was: The only change I made to that was to add a little more detail that is specific only to this notification: |
Everything on that page is referred to as an alert, simply to alert the user (mechanism may be via android notification) You can decapitalize the Expiry to be consistent with the others. By specifying the time periods when the alerts would happen means we can't change that without forcing translators to update every language. Its better not to specify it there. The main reason I didn't already extract the strings is because I thought the wording might change and also the feature was untested. |
Makes sense. Thanks. I have converted to draft so we can edit when it has been tested. And thanks for reminding me, again, that numbers should not be included in strings. Sorry about that. I am sorry I still don't understand why we should refer to it as an alert. It will never ever make a sound unless I have completely misunderstood the behavior. |
@jamorham On that same page, there is one item that is titled "Forecast Low". The way I look at that page, I see the title of the page is "Extra Alerts". That tells me everything here is an alert unless otherwise specified. So, Forecast Low is about a Forecast low alert even though the word alert is not included. I know that because of the title of the page. I hope the following analogy is not offensive. He decides to ask a question from his employer, the owner of the shop. He says "we only sell paint here; why do we need to include the word paint on every label". I'm that student! :o) |
710e0c7
to
23ccbd1
Compare
…3_02_14 # Conflicts: # app/src/main/res/values/strings.xml
The setting can now be translated.
I have changed the title to "Sensor expiry". My thinking is that we need to minimize the length of the titles as they don't wrap. And everything on this menu is related to notifications (and alerts) anyway. So, my view is that there is no need to include notification or alert in the title.
I have changed the summary to state the timing. We can say that this is for documentation. I agree that there are things that should go into documentation rather than the menu. But, this is not too long and provides relevant info to the user.