-
-
Notifications
You must be signed in to change notification settings - Fork 102
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
Possibility to add an alert for iOS #58
base: master
Are you sure you want to change the base?
Conversation
Fell free to tell me if there's anything I forgot, or if you meet any troubles ! |
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.
Hi! Thanks for your time and your contribution. I have some comments regarding how this is implemented. Also please note your implementation allows only single reminder while there can be multiple defined.
I suggest you take a look at react-native-calendar-events which already offers this functionality and get inspired there. I think there is no need to reinvent the wheel - imo you can just copy the relevant code since the license allows to do so, and we'll credit the author in release notes.
- If you set `alert: "1"` => will add an alert 5 minutes before the startDate. | ||
- If you set `alert: "2"` => will add an alert 30 minutes before the startDate. | ||
- If you set `alert: "3"` => will add an alert 60 minutes before the startDate. | ||
- If alert is not set in the eventConfig, no alert will be set. |
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.
The options "0" ... "3" seem rather random - why not just pass the number of minutes from the JS side?
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.
Ahh, yeah I was quite agree but because I didn't see that react-native-calendar-events already managed that. I'll have a look to see how I can eventually implement it here ! Thank's !
|
||
if ([[RCTConvert NSString:options[_alert]] caseInsensitiveCompare:@"0"] == NSOrderedSame) | ||
{ | ||
EKAlarm * alarm = [EKAlarm alarmWithAbsoluteDate:originalDate]; |
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.
you're using alarmWithAbsoluteDate
- let's consider the following scenario: user
- creates an event for 2pm and adds an alert that should fire 30 minutes before
- moves the event to 1:30pm
- when does the alert fire and is it what the user expects?
I think this might be useful.
{ | ||
NSDate *alertReminder = [originalDate dateByAddingTimeInterval:-60*60]; | ||
EKAlarm * alarm = [EKAlarm alarmWithAbsoluteDate:alertReminder]; | ||
event.alarms = @[alarm]; |
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.
take a good look at the code: there is a lot of duplicates - the ifs look more or less the same, with just small differences. It would be better to remove the duplications and perhaps extract this functionality into a separate function :)
Resolve conflicts and add functionality, please! |
yes please resolve the conflicts and merge ! |
Just wanted to share what is working for me to add alerts to iOS. As suggested above I'm using
|
@jordanwade would you be willing to open a PR? it'd be much appreciated, thanks! |
This works nicely on my testing as well, please open a PR :) Also, would you mind implementing this to Android as well? Thanks! |
hello @Jbarget and thanks for commenting, as explained in the readme, I no longer actively develop the package. If you want to add new features, you can hire me, as explained https://github.com/vonovak/react-native-add-calendar-event#maintenance-notice thank you |
Resolve conflicts and add functionality, please! |
I added the possibility to set an alarm since there was none by default. It works for iOS, because in Android, it use the native calendar, and users already got reminder 5 or 10 minutes before any events on their calendar, but not on iOS if no alarm is set.
I just added a field alert, and manage the differences in
ios/AddCalendarEvent.m
and succeed to manage it thanks toEKAlarm
.You just have to pass a field
alert
in the eventConfig.