Skip to content
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

Feat: Allow for repeat to specify amount of the given repeat type (Android) #2030

Merged
merged 6 commits into from
Jun 24, 2021
Merged

Feat: Allow for repeat to specify amount of the given repeat type (Android) #2030

merged 6 commits into from
Jun 24, 2021

Conversation

johgusta
Copy link
Contributor

Problem

We want our local notifications to be able to repeat every other day, and it is critical that the time of day is correct. Currently the closest possible setting would be to use repeatType: 'time' and then calculate the milliseconds for two days and set it as repeatTime. For a repeating reminder this will change the time of day the reminder repeats when daylight saving changes the UTC offset.

Solution

Since the library already allows repeatTime for time, this PR will update it to allow repeatTime for all the repeat types. Instead of calculating the milliseconds of each type we're using the Calendar.add() function with the corresponding field. The amount to increase it by is set by repeatTime and defauls to 1. It also follows the same logic as already implemented for repeating on months.

Example of how to repeat every other day:

PushNotification.localNotificationSchedule({
    ...
    repeatType: 'day',
    repeatTime: 2,
    ...
});

yepes and others added 4 commits May 19, 2021 16:44
Fixed typos
This allows repeat interval to be configured as following
"Every 3 days": {repeatType: "day", repeatTime: 3}
"Every 2 weeks": {repeatType: "week", repeatTime: 2}
README.md Outdated
@@ -324,6 +324,7 @@ PushNotification.localNotification({
ignoreInForeground: false, // (optional) if true, the notification will not be visible when the app is in the foreground (useful for parity with how iOS notifications appear). should be used in combine with `com.dieam.reactnativepushnotification.notification_foreground` setting
shortcutId: "shortcut-id", // (optional) If this notification is duplicative of a Launcher shortcut, sets the id of the shortcut, in case the Launcher wants to hide the shortcut, default undefined
onlyAlertOnce: false, // (optional) alert will open only once with sound and notify, default: false
repeatTime: 1, // (optional) Increment of configured repeateType. Check 'Repeating Notifications' section for more info.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @johgusta
This line should not be documented there, it's on the scheduldedNotification method 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might have misunderstood this, but I think repeating is possible for both the localNotification and localNotificationSchedule functions. Since this one has the documentation for repeatType: 'day' I thought this would be the right place to add this info. In my project we only use localNotificationSchedule though but as far as I can tell it should still work for both.

@Dallas62
Copy link
Collaborator

Hi @johgusta
Thanks for the PR, can you take a look at comments ?
I will test the PR just after changes 😉
Regards

@Dallas62 Dallas62 changed the base branch from master to dev June 19, 2021 18:51
@johgusta
Copy link
Contributor Author

Hi @johgusta
Thanks for the PR, can you take a look at comments ?
I will test the PR just after changes 😉
Regards

Thanks for the review! I've updated the code and I think the documentation is correct as it is, but let me know if I've misunderstood that and I'll fix it.

@johgusta johgusta requested a review from Dallas62 June 24, 2021 13:45
@johgusta
Copy link
Contributor Author

Hi @johgusta
Thanks for the update, the issue with the documentation is that there is repeatTime added at a wrong place for localNotification, this option is not available for this method

Ok! I will update the documentation and move it to the the scheduled notification section then.

@Dallas62 Dallas62 merged commit 100e8f4 into zo0r:dev Jun 24, 2021
@Dallas62
Copy link
Collaborator

Thanks for your contribution !
Will take a look ASAP and release it.

@johgusta johgusta deleted the repeat-time-for-all-repeat-types branch June 28, 2021 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants