-
Notifications
You must be signed in to change notification settings - Fork 17
G7SensorKit: Update expiration timer #11
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
Conversation
moved expiration timer to DateComponentsFormatter to allow more granular control over date units displayed
Please describe what the problem being fixed here is.
Again, please describe why this is an issue and what is being changed to fix it. |
|
|
||
| formatter.dateStyle = .short | ||
| formatter.timeStyle = .short | ||
| formatter.dateFormat = "E, MMM d, h:mm a" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We prefer not to specify date formats, so we can benefit from built in iOS locale support. Some locales do not prefer am/pm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the use of the following satisfy this?:
formatter.locale = Locale.current
formatter.setLocalizedDateFormatFromTemplate("E, MMM d, hh:mm")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work on small screens? Can you please check on iPhone SE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have an actual SE device to test this on. Simulator doesn't connect to real G7. Open to suggestions or help on how to test on SE.
Set locale and use setLocalizedDateFormatFromTemplate in date formatter for sensor date displays
|
I've updated the description and pushed a commit to correct (hopefully) the localization issue. |
| .foregroundColor(color(for: viewModel.progressBarState.labelColor)) | ||
|
|
||
| Spacer() | ||
| var sessionLengthFormatter: DateComponentsFormatter = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
View composition shouldn't have significant blocks of non-view code; it makes it harder to see what is layout and what is logic. And the old durationFormatter is still left in the code, unused. Probably better to move the contents of this formatter block up to where durationFormatter was defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this. Moved sessionLengthFormatter out of view. Removed durationFormatter.
Moved sessionLengthFormatter out of view Removed original durationFormatter
|
Thanks! |
Problem 1:
G7 Setting screen shows sensor expiration time as too broad in some situations. (ie. shows "Expires in 1 week" when there are in fact "9 days, 23 hours" remaining
Solution 1:
Update date formatter of sensor expiration timer to use DateComponentsFormatter, formatter.allowedUnits, and formatter.maximumUnitCount to display 2 more appropriate granular date units for more accuracy (ie. "Sensor expires - 8 days, 22 hours" or "4 hours, 27 minutes")
Problem 2:
G7 Setting screen Sensor Start, Sensor Expiration, and Grace Period End displays are a bit difficult to read at a glance as they do not show a day of week or (in the case of locales where it's the standard) AM or PM.
Solution 2:
Update date/time display formatter to include an abbreviated day of week, as well as appropriate 24H or AM/PM display through the setting of formatter.locale, and use of setLocalizedDateFormatFromTemplate to ensure localized display of date and time (setLocalizedDateFormatFromTemplate ensures day and month display in preferred order as well as handles 24hr vs AM/PM display)