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.

Sign up for GitHub

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

Jump to bottom

feat(local-notifications): Support setting seconds in Schedule "on" #253

Merged
merged 4 commits into from
Feb 17, 2021
Merged

feat(local-notifications): Support setting seconds in Schedule "on" #253

merged 4 commits into from
Feb 17, 2021

Conversation

theproducer
Copy link
Contributor

This PR adds the ability to define seconds on the on property of LocalNotificationSchedule.

fixes: ionic-team/capacitor#1679

@theproducer theproducer mentioned this pull request Feb 15, 2021
Merged
jcesarmobile
jcesarmobile approved these changes Feb 17, 2021
View reviewed changes
Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

Looks good, but I think this should be a feature, not a fix, despite it fixes an issue too, the main change is to allow passing seconds (and involves changing the types)

@theproducer theproducer changed the title fix(local-notifications): Support setting seconds in Schedule "on" feat(local-notifications): Support setting seconds in Schedule "on" Feb 17, 2021
@theproducer theproducer merged commit 4ec8d06 into main Feb 17, 2021
@theproducer theproducer deleted the local-notifications-add-seconds branch February 17, 2021 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlpoole carlpoole carlpoole approved these changes

@jcesarmobile jcesarmobile jcesarmobile approved these changes

@imhoffd imhoffd Awaiting requested review from imhoffd

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

LocalNotifications: add second option to on pattern
3 participants
@theproducer @jcesarmobile @carlpoole