-
Notifications
You must be signed in to change notification settings - Fork 628
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
Added functionality to push daily reminder notification everyday at 1… #3322
Conversation
…2AM urging to fill in expenses ,transactions and incomes;
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.
- Fill the PR template
- Revert unnecessary changes
- Extract the reminders feature as a separate module
- Make the reminder a Setting that users can turn off
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.
Revert this change
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.
Revert these changes
} | ||
//For pushing Notification to the user as a reminder at every 12AM | ||
// urging to fill in expenses, transactions and incomes | ||
fun createNotificationChannel(context: Context) { |
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.
Extract this logic in a separate class and module that lives outside :app
. For example, :feature:reminders
@@ -99,6 +99,10 @@ class RootActivity : AppCompatActivity(), RootScreen { | |||
setupDatePicker() | |||
setupTimePicker() | |||
|
|||
createNotificationChannel(this) |
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.
Not all users might want to have reminders, use IvyFeatures
to make it a customizable setting that's enabled by default
if (ActivityCompat.checkSelfPermission( | ||
context, | ||
Manifest.permission.POST_NOTIFICATIONS | ||
) != PackageManager.PERMISSION_GRANTED |
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 code would simpler if:
- Extract the permission check as a
hasNotificationsPermission
function - if (hasNotificationsPermisson()) notify
|
||
val builder = NotificationCompat.Builder(context, "DAILY_REMINDER_CHANNEL_ID") | ||
.setSmallIcon(R.drawable.ic_launcher-playstore) | ||
.setContentTitle("EveryDay Reminder") |
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.
Extract these a string so they can be localized
Overall, the implementation looks good and should work but please attach a screen recording testing it |
val builder = NotificationCompat.Builder(context, "DAILY_REMINDER_CHANNEL_ID") | ||
.setSmallIcon(R.drawable.ic_launcher-playstore) | ||
.setContentTitle("EveryDay Reminder") | ||
.setContentText("Fill in todays expences,transactions and incomes.") |
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.
typo in expenses
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.
Thanks for reviewing @suyash01 🙌
…2AM urging to fill in expenses ,transactions and incomes;
Pull request (PR) checklist
Please check if your pull request fulfills the following requirements:
main
branch.Screen recording of interacting with your changes:
What's changed?
Describe with a few bullets what's new:
...
...
Risk factors
What may go wrong if we merge your PR?
In what cases won't your code work?
Does this PR close any GitHub issues?
Troubleshooting CI failures ❌
Pull request checks failing? Read our CI Troubleshooting guide.