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

Allow sensor expiry notification setting to be translated #2677

Merged
merged 3 commits into from
Jul 30, 2023

Conversation

Navid200
Copy link
Collaborator

@Navid200 Navid200 commented Feb 14, 2023

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.

@Navid200 Navid200 changed the title Allow sensor expiry notifications setting to be translated Allow sensor expiry notification setting to be translated Feb 15, 2023
@jamorham
Copy link
Collaborator

This doesn't preserve the existing text strings.

@Navid200
Copy link
Collaborator Author

@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.
I thought I improved the existing strings. Please see below.


Title

The title was:
Sensor Expiry Alerts

But, it's not an alert. it is a notification, isn't it?
So, I changes the title to:
Sensor expiry

Why didn't I call it Sensor expiry notification?
This page contains alerts only. Now, it is going to contain alerts and notifications. The user needs to know what it is. Is it sensor expiry, persistent high BG, forecast low BG? Whether it is an alert or a notification is not something the user can choose.
I mean we don't have a sensor expiry notification as well as a sensor expiry alert.

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.
So, I didn't think we should include alert or notification in the title for that reason.


Summary

The summary was:
Raise a notification when the sensor gets close to expiry.

The only change I made to that was to add a little more detail that is specific only to this notification:
Raise notifications when approaching (24, 9 and 2 hours) sensor expiry.

@jamorham
Copy link
Collaborator

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.

@Navid200 Navid200 marked this pull request as draft February 22, 2023 18:40
@Navid200
Copy link
Collaborator Author

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.

@Navid200
Copy link
Collaborator Author

@jamorham On that same page, there is one item that is titled "Forecast Low".
That's the only one that makes sense to me.

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.
A student starts a summer job in a paint store and sees the following as the labels on the shelves of paint cans:
Red paint, blue paint, green paint, black paint, white paint, yellow paint, brown paint, pink paint, orange paint, ...

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)

@Navid200 Navid200 closed this Jul 26, 2023
@Navid200 Navid200 force-pushed the Navid_2023_02_14 branch 2 times, most recently from 710e0c7 to 23ccbd1 Compare July 26, 2023 12:31
@Navid200 Navid200 reopened this Jul 26, 2023
@Navid200 Navid200 marked this pull request as ready for review July 26, 2023 12:42
@jamorham jamorham merged commit 2dd482e into NightscoutFoundation:master Jul 30, 2023
2 checks passed
@Navid200 Navid200 deleted the Navid_2023_02_14 branch August 9, 2023 23:32
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.

2 participants