Style local notifications#19 #42
Conversation
- Added a content closure parameter to customize notification content creation - Updated Scheduler initialization to include the new content closure argument
There was a problem hiding this comment.
Thank you @AballahNsour for your PR and contributing to the Spezi ecosystem. Sorry for the delay in reviewing the PR and providing some first feedback!
I had some comments thought the PR that would be great to be addressed. In addition, please fill out the PR template to describe the change and relevant context for the PR.
Two major elements that would also require attention would be:
- Add one or more tests to validate that the styling functionality is working as expected or at least that a styling closure is called as expected and as often as expected.
- Add some documentation in the DocC documentation and added public interface to point people to the functionality including some examples that are always helpful for beginners to look at and guide them to the right direction.
In general, we should consider how we want to drive this framework forward in general (see #20) but great to see this land in this PR and further contribute to a version of the scheduler closer to a 1.0 of the package!
Thanks for the first contribution and it would be great to learn what you are building with Spezi; always feel free to share it at https://github.com/orgs/StanfordSpezi/discussions/categories/show-and-tell.
| content = UNMutableNotificationContent() | ||
| content.title = title | ||
| content.body = description | ||
|
|
| content.body = description | ||
| let content: UNMutableNotificationContent | ||
| if let contentClosure = contentClosure { | ||
| content = contentClosure(self, event) as! UNMutableNotificationContent |
There was a problem hiding this comment.
A force cast here could fail if the user does't return a mutable notification context. We should rather assume that a user returns the UNNotificationContent and rather create a mutable context below and then assign it to content which should be of the type UNNotificationContent
| @@ -54,15 +55,16 @@ public class Scheduler<Context: Codable>: Module, EnvironmentAccessible, Default | |||
| /// We recommend setting the limit to a value lower than 64, e.g., 56, to ensure room inaccuracies in the iOS scheduling APIs. | |||
| /// The default value is `56`. | |||
| /// - Parameter initialTasks: The initial set of ``Task``s. | |||
| public init(prescheduleNotificationLimit: Int = 56, tasks initialTasks: [Task<Context>] = []) { | |||
| public init(prescheduleNotificationLimit: Int = 56, tasks initialTasks: [Task<Context>] = [], notificationContentClosure: ((Task<Context>, Event) -> UNNotificationContent)? = nil) { | |||
There was a problem hiding this comment.
I would generally suggest to start adding multi-line arguments if we have longer function signatures. The double-space before the notificationContentClosure triggers a SwiftLint warning. Feel free to run swiftlint locally on your device to see the current set of warnings that will also be triggered by the automated PR checks.
| public init(prescheduleNotificationLimit: Int = 56, tasks initialTasks: [Task<Context>] = [], notificationContentClosure: ((Task<Context>, Event) -> UNNotificationContent)? = nil) { | |
| public init( | |
| prescheduleNotificationLimit: Int = 56, | |
| tasks initialTasks: [Task<Context>] = [], | |
| notificationContentClosure: ((Task<Context>, Event) -> UNNotificationContent)? = nil | |
| ) { |
| @@ -39,6 +39,7 @@ public class Scheduler<Context: Codable>: Module, EnvironmentAccessible, Default | |||
| taskList.tasks | |||
| } | |||
|
|
|||
| public var notificationContentClosure: ((Task<Context>, Event) -> UNNotificationContent)? | |||
There was a problem hiding this comment.
Wondering if we should make this public and modifiable after the initial configuration. If we make it publicly modifiable we would need to re-pass this back to all of the current existing tasks in a didSet clause.
| XCTAssertEqual(scheduledEventHash, completedEventHash) | ||
| } | ||
| func testTaskSchedulingWithStartDateAndEndDate() async throws { |
There was a problem hiding this comment.
Thank you for adding some tests here. As we already work on these in #41 it might make sense to rather only include it there and include a test there that validates that the notification styling functionality is called correctly and that the content is used to then schedule a notification.
Name of the PR
♻️ Current situation & Problem
Link any open issues or pull requests (PRs) related to this PR. Please ensure that all non-trivial PRs are first tracked and discussed in an existing GitHub issue or discussion.
⚙️ Release Notes
Add a bullet point list summary of the feature and possible migration guides if this is a breaking change so this section can be added to the release notes.
Include code snippets that provide examples of the feature implemented or links to the documentation if it appends or changes the public interface.
📚 Documentation
Please ensure that you properly document any additions in conformance to Spezi Documentation Guide.
You can use this section to describe your solution, but we encourage contributors to document your reasoning and changes using in-line documentation.
✅ Testing
Please ensure that the PR meets the testing requirements set by CodeCov and that new functionality is appropriately tested.
This section describes important information about the tests and why some elements might not be testable.
📝 Code of Conduct & Contributing Guidelines
By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: