Add ability to repeat notifications like on iOS #268

ghivert · 2021-03-11T21:31:17Z

Hi,

Today the repeats option does not work. This PR fix this, and add the ability to freely choose the repeating interval, like asking in #263.
The idea is rather simple: let the user choose how they want to repeat the notifications with a repeatsComponent field in the request, and converting the components to NSCalendarUnit to have the exact same behavior in JS than what we have in Objective-C.
In case there’s no repeats, then the old behavior is used, with the same NSCalendarUnit to ensure the notification will be sent at the correct date.

Tell me if it suits you. If not, I would be happy to fix what you want. :)

P0SEID0N · 2021-06-07T16:45:21Z

Any update on getting this into the build? I am working on a project where we need access to weekly repeat intervals. I would prefer to be able to schedule notifications locally rather than needing to rely on internet connection to compensate with a server backend.

Thanks!

ghivert · 2021-06-08T14:16:27Z

I personnaly ended up with my personal repo, for my own needs while the maintainers are not available. The package is perfectly working on my app fyi, with the notifications displayed at the correct time and hour. The code modification is really small, so you can easily drop it when the bug will be fixed in the future.

iamharky · 2021-06-25T20:50:55Z

Thank you so much @ghivert !
Your solution seems to work for me, perfectly repeats notifications.

batuhansahan · 2021-07-03T23:08:06Z

Any update on getting this into the build? @Dallas62 @Naturalclar

Dallas62 · 2021-07-04T05:58:55Z

Hi,
I'm not the maintainer of this repository. But I do not understand why we should be available to repeat the notification from nanoseconds ?
And why can we repeat the notification by month and hour ? If it's hourly, by definition it will go through months and days.
A unique type of repeating should be enough.
Regards

smadan · 2021-07-08T22:43:15Z

Pinging this again - I want to use this for my app. Can one of the maintainers approve this change? @Naturalclar?

ghivert · 2021-07-09T12:38:31Z

Hi,
I'm not the maintainer of this repository. But I do not understand why we should be available to repeat the notification from nanoseconds ?
And why can we repeat the notification by month and hour ? If it's hourly, by definition it will go through months and days.
A unique type of repeating should be enough.
Regards

@Dallas62 Hi,
No problem with your questions, I had the same interrogations when writing the PR. The best you can do is playing with the iOS API. It's not really clear at first sight why it's doing, but it allows you to be really precise for your notifications. You can for example specify the hours for specific months. That's why you can use months and hours.

My idea was to mimic the behavior provided by iOS in this package, because we're only dealing with iOS and it would be too bad to ignore some features because of some reasons. You can easily write a wrapper talking both to iOS and Android with this package as foundation. Of course I'm open to debate, but I think keeping this feature as a wrapper is better than providing a custom solution: I really don't know the different use cases for notifications and it's probably easy to forget about some users.

Pinging this again - I want to use this for my app. Can one of the maintainers approve this change? @Naturalclar?

@smadan The package hadn't changed since I wrote the PR, so you can probably use ghivert/push-notification-ios#2b9a66b in your package.json while the PR is pending, and switch back to the package when the merge is done. Your only risk is to have some API changes when the PR will be merged, but if you need this for work, this can be worth it.

P0SEID0N · 2021-07-09T22:00:26Z

Hi,
I'm not the maintainer of this repository. But I do not understand why we should be available to repeat the notification from nanoseconds ?
And why can we repeat the notification by month and hour ? If it's hourly, by definition it will go through months and days.
A unique type of repeating should be enough.
Regards

I agree that nanoseconds are not really necessary as there really shouldnt be any situations where you're emitting notification events in nanosecond intervals.

On your comment about month and hour. I agree somewhat, but if you're thinking logistically about it, it is possible to have a notification go off at specific hours but not recast that notification by the month. For example if you cast by Month and Hour there might be situations where your notifications do not emit at the expected time. However all of these things are edge cases that can easily be avoided with some very lightweight code. However I see what the original PR creator was trying to do, I have done some digging myself into IOS notifications and they did mimic how IOS's OS handles notifications which I can respect (keeping the fundamentals the same that is).

I do not disagree that we can optimize the code here though. @Dallas62 Do you plan on use this PR for your notification library? I was having an issue with repeating notifications and had planned to manually change your own Package to use this fork (or a similar edit). Repeating intervals is somewhat important for timers and such.

jamesxabregas · 2021-07-20T03:44:54Z

So I wish I has come across this PR before I spent the time on implementing much the same thing... I just submitted a PR recently #308. Your approach seems to be a bit more configurable, I just sought to replicate the repeatInterval functionality that is available in scheduleLocalNotification().
It would be good if the project maintainer could take a look at these PRs. At the moment the notification does not repeat at all despite what the documentation says.

jamesxabregas · 2021-07-20T03:51:48Z

You should probably also add in NSCalendarUnitWeekday as one of the NSDateComponents as scheduling reoccuring weekly notifications is a fairly common requirement.

jamesxabregas

Overall I think this is good. My suggestions would be to also allow repeating by year and dayOfWeek, but to remove nanosecond. Even if nanosecond repeats are supported in iOS, I'm not sure if this library should support it.

jamesxabregas · 2021-07-20T08:39:23Z

README.md

 - `sound` : The sound played when the notification is fired.
 - `category` : The category of this notification, required for actionable notifications.
 - `isSilent` : If true, the notification will appear without sound.
 - `userInfo` : An object containing additional notification data.

+request.repeatsComponent is an object containing (each field is optionnal):
+
+- `month`: Will repeat every selected month in your fireDate.


Suggest adding year component.

jamesxabregas · 2021-07-20T08:40:03Z

README.md

+request.repeatsComponent is an object containing (each field is optionnal):
+
+- `month`: Will repeat every selected month in your fireDate.
+- `day`: Will repeat every selected day in your fireDate.


Suggest adding dayOfWeek component

jamesxabregas · 2021-07-20T08:40:28Z

README.md

+- `hour`: Will repeat every selected hour in your fireDate.
+- `minute`: Will repeat every selected minute in your fireDate.
+- `second`: Will repeat every selected second in your fireDate.
+- `nanosecond`: Will repeat every selected nanosecond in your fireDate.


Suggest removing nanosecond

jamesxabregas · 2021-07-20T08:43:43Z

ios/RCTConvert+Notification.m

+        NSCalendarUnitHour |
+        NSCalendarUnitMinute |
+        NSCalendarUnitSecond;
+    NSCalendarUnit repeatDateComponents =


Suggest addition of NSCalendarUnitYear and NSCalendarUnitWeekday.
Suggest removal of NSCalendarUnitNanosecond.

jamesxabregas · 2021-07-20T08:45:01Z

js/types.js

+   * Must be used with repeats and fireDate.
+   */
+  repeatsComponent?: {
+    month?: boolean,


Suggest addition of year and dayOfWeek. Suggest removal of nanosecond.

Naturalclar · 2021-07-27T06:09:34Z

@ghivert @jamesxabregas
Hello, I'm the maintainer of this library, I'm so sorry for not being able to respond earlier.

This would be a great addition, and would love to merge this in.
@ghivert, could you apply the suggestions by @jamesxabregas ? Once that's done I'll do some testing and publish the library

Overall I think this is good. My suggestions would be to also allow repeating by year and dayOfWeek, but to remove nanosecond. Even if nanosecond repeats are supported in iOS, I'm not sure if this library should support it.

I agree that nanosecond should be removed as well

ghivert · 2021-07-28T14:08:28Z

Hi,
I'll do the change, but I can't do them right now. I can find some time during to next one or two weeks.

Naturalclar · 2021-07-29T12:38:10Z

No rush 😁
Thanks so much!

batuhansahan · 2021-08-12T15:49:09Z

@ghivert Thanks so much!!!

ios/RCTConvert+Notification.m

Naturalclar

@ghivert thanks so much for the change!
I will apply suggestion from @jamesxabregas in a separate PR and publish a new version.

…tification#268) Co-authored-by: Jesse Katsumata <jesse.katsumata@gmail.com> Co-authored-by: Jesse Katsumata <niconico.clarinet@gmail.com>

ghivert added 3 commits March 11, 2021 22:19

🐛 Add repeats component to addNotificationRequest

8738e6d

📝 Write docs for repeating

85cad5a

🐛 Fix dateRepeatsComponent in types.js

2b9a66b

batuhansahan approved these changes Jul 3, 2021

View reviewed changes

jamesxabregas suggested changes Jul 20, 2021

View reviewed changes

Merge branch 'master' into master

82e3abb

Naturalclar reviewed Aug 20, 2021

View reviewed changes

ios/RCTConvert+Notification.m Outdated Show resolved Hide resolved

Apply suggestions from code review

ed38879

Naturalclar approved these changes Aug 20, 2021

View reviewed changes

Naturalclar merged commit b28a99c into react-native-push-notification:master Aug 20, 2021

This was referenced Aug 20, 2021

feat: add day of the week and year repetition #325

Merged

Support repeatIntervals of minute, hour, day, week, month, and year in addNotificationRequest() #308

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to repeat notifications like on iOS #268

Add ability to repeat notifications like on iOS #268

ghivert commented Mar 11, 2021

P0SEID0N commented Jun 7, 2021

ghivert commented Jun 8, 2021

iamharky commented Jun 25, 2021

batuhansahan commented Jul 3, 2021

Dallas62 commented Jul 4, 2021

smadan commented Jul 8, 2021

ghivert commented Jul 9, 2021

P0SEID0N commented Jul 9, 2021

jamesxabregas commented Jul 20, 2021

jamesxabregas commented Jul 20, 2021

jamesxabregas left a comment

jamesxabregas Jul 20, 2021

jamesxabregas Jul 20, 2021

jamesxabregas Jul 20, 2021

jamesxabregas Jul 20, 2021

jamesxabregas Jul 20, 2021

Naturalclar commented Jul 27, 2021

ghivert commented Jul 28, 2021

Naturalclar commented Jul 29, 2021

batuhansahan commented Aug 12, 2021

Naturalclar left a comment

Add ability to repeat notifications like on iOS #268

Add ability to repeat notifications like on iOS #268

Conversation

ghivert commented Mar 11, 2021

P0SEID0N commented Jun 7, 2021

ghivert commented Jun 8, 2021

iamharky commented Jun 25, 2021

batuhansahan commented Jul 3, 2021

Dallas62 commented Jul 4, 2021

smadan commented Jul 8, 2021

ghivert commented Jul 9, 2021

P0SEID0N commented Jul 9, 2021

jamesxabregas commented Jul 20, 2021

jamesxabregas commented Jul 20, 2021

jamesxabregas left a comment

Choose a reason for hiding this comment

jamesxabregas Jul 20, 2021

Choose a reason for hiding this comment

jamesxabregas Jul 20, 2021

Choose a reason for hiding this comment

jamesxabregas Jul 20, 2021

Choose a reason for hiding this comment

jamesxabregas Jul 20, 2021

Choose a reason for hiding this comment

jamesxabregas Jul 20, 2021

Choose a reason for hiding this comment

Naturalclar commented Jul 27, 2021

ghivert commented Jul 28, 2021

Naturalclar commented Jul 29, 2021

batuhansahan commented Aug 12, 2021

Naturalclar left a comment

Choose a reason for hiding this comment