- Clock Widget: fix bug, better default Handling for: use12Hour, showSeconds#982
- Clock Widget: fix bug, better default Handling for: use12Hour, showSeconds#982Lissy93 merged 4 commits intoLissy93:masterfrom Totto16:master
Conversation
✄1�7 Deploy Preview for dashy-dev ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
Changes preview: Legend: |
| return !this.options.hideSeconds; | ||
| if (this.options.hideSeconds) return !this.options.hideSeconds; | ||
| // this is the default | ||
| return true; |
There was a problem hiding this comment.
Since the ! is converting to a bool as well as negating, there shouldn't be a need to explicitly return true
There was a problem hiding this comment.
I know, but its more cleaner and the default is clearly visible, the main question here is, can this defaukt change, here I think it wan't so I agree that the ! is cleaner.
| return !this.options.use12Hour; | ||
| if (this.options.use12Hour) return this.options.use12Hour; | ||
| // this is the default, it gets computed by the DateTimeFormat implementation | ||
| return Intl.DateTimeFormat(this.timeFormat, { timeZone: this.timeZone, hour: 'numeric' }).resolvedOptions().hour12 ?? false; |
| }, | ||
| use12Hour() { | ||
| return !this.options.use12Hour; | ||
| if (this.options.use12Hour) return this.options.use12Hour; |
There was a problem hiding this comment.
If the user set this.options.use12Hour: false (because they wanted 24-hr), then the first statement would skip, and would user the systems current time. I think to continue with this approach, we'd need to explicitly check for a bool (e.g. typeof this.options.use12Hour == "boolean")
Lissy93
left a comment
Lissy93
left a comment
There was a problem hiding this comment.
Looks good - thanks for this :)
2 participants
Category:
Overview
The previous version of the function
use12Hourwas implemented wrong, it didn't separate the cases, where the options was set or unset. I fixed the behavior and added a lookup in the case, it's unset, now it behaves as expected.I used the function resolvedOptions() from the DateTimeFormat API, to determine if it's using the 12 Hour format, according to
timeZoneandformat.Additionally I simplified the function
showSeconds, so that the default is set explicitly determined, and not implictly true.Code Quality Checklist (Please complete)