-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature/87 add event reminder #91
base: temp/merge-write-calendar
Are you sure you want to change the base?
Feature/87 add event reminder #91
Conversation
Also don't try to set up a null event.
#68 - Avoid abbreviations where possible
#62 rename cursor to something more meaningful
…nsion RoundToNearestMinutes
…-rename #59 rename extensions to more accurately represent their purpose
…th-any #61 replace usages of .Count == 0 with !.Any()
…d-deleted-flag #66 Remove not needed Deleted flag from create+update event
EKAlarm.FromTimeInterval(-((double)calendarEventReminder.MinutesPriorToEventStart * 60)) | ||
}; | ||
|
||
if (CalendarRequest.Instance.SaveEvent(calendarEvent, EKSpan.FutureEvents, true, out var error)) |
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.
use return instead of if
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.
Altered accordingly
|
||
calendarEvent.Alarms = null; | ||
|
||
if (CalendarRequest.Instance.SaveEvent(calendarEvent, EKSpan.FutureEvents, true, out var error)) |
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.
as above comment
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.
Altered accordingly
StartDate = ViewModel.StartDate, | ||
Title = ViewModel.Title | ||
}; | ||
ViewModel.Attendees = lst; |
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.
Investigate why we're manually updating bindingcontext instead of just binding to the viewmodel
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.
Altered to use viewmodel instead of binding to API model
uwpAppointment.Reminder = TimeSpan.FromMinutes(calendarEventReminder.MinutesPriorToEventStart); | ||
var calendar = await instance.GetAppointmentCalendarAsync(uwpAppointment.CalendarId); | ||
await calendar.SaveAppointmentAsync(uwpAppointment); | ||
uwpAppointment = await instance.GetAppointmentAsync(eventId); |
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.
Don't feel this is necessary - should wrap SaveAppointmentAsync in try catch and return false from catch if can't save
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.
After looking at how I was handling CalendarEvents, I believe when you 'save' the uwpAppointment it will actually update the event you pass in, so it might just be worth checking that uwpAppointment.Reminder != null as the return. (without the reget)
see:
await calendar.SaveAppointmentAsync(appointment);
if (!string.IsNullOrEmpty(appointment.LocalId))
return appointment.LocalId;
{ | ||
if (string.IsNullOrWhiteSpace(eventId)) | ||
{ | ||
throw new ArgumentException($"[UWP]: No Event found for event Id {eventId}"); |
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.
Let's pass the native exception into the new Exceptions we're creating here
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.
Will be doing this in a separate PR after this one
No description provided.