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

Added Notifications with repeating Schedules Solves #55 #57

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

SalikSayyed
Copy link

@SalikSayyed SalikSayyed commented Aug 24, 2021

Description

Build

  • notificationHandler
  • notificationSetter
  • WeekDaySelector
    Used Expo local notifications.

fix #55

Ticket Link

Closes #55

How has this been tested?

Pixel 2 API 29 - Simulator

Screenshots

image
image

image

image

image

image

image

Checklist

  • Added a "Closes [issue number]" in the ticket link section
  • You have not changed whitespace unnecessarily (it makes diffs hard to read)
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens

@SalikSayyed SalikSayyed changed the title Contribution to Enhancement #55 Added Notifications with repeating Schedules #55 Aug 24, 2021
@SalikSayyed SalikSayyed changed the title Added Notifications with repeating Schedules #55 Added Notifications with repeating Schedules Solves #55 Aug 24, 2021
@watadarkstar
Copy link
Collaborator

watadarkstar commented Aug 24, 2021

Nice work @SalikSayyed! I will review the code soon. In terms of the UI, what about placing the weekday / time part inside a modal that pops up instead of placing it inside the list? You can use react-native-paper's Dialog component for the modal and put it in its own component like NotificationDialog :) This will make things easier to move around in the future.

https://callstack.github.io/react-native-paper/dialog.html

SalikSayyed and others added 3 commits August 25, 2021 00:22
@watadarkstar watadarkstar self-requested a review August 24, 2021 19:05
Copy link
Collaborator

@watadarkstar watadarkstar left a comment

Choose a reason for hiding this comment

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

Done reviewing for now! Good work. Let me know when you make the requested changes and I can review it again. Please update the screenshots once you turn it into a Dialog.

package.json Outdated Show resolved Hide resolved
yarn-error.log Outdated Show resolved Hide resolved
@watadarkstar
Copy link
Collaborator

Nice work @SalikSayyed, I left a few more comments and then this should be good!

@SalikSayyed
Copy link
Author

The toast library is giving me this issue, do I have to run pod install? In any case, let's stick with the libraries we already have installed and use https://callstack.github.io/react-native-paper/snackbar.html.

simulator_screenshot_5D3DFD63-BFD1-48BD-8285-3E975A13C986

Changed to SnackBar. no new package needed

@SalikSayyed
Copy link
Author

Hello, @watadarkstar will u please review this ?

interface Props {
message: string
show: boolean
changeShow: React.Dispatch<React.SetStateAction<boolean>>
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: call this setShow

@watadarkstar
Copy link
Collaborator

watadarkstar commented Sep 22, 2021

@SalikSayyed Sure reviewing it now :) Thank you for reminding me!

The UI is a bit odd. The pick time and notify buttons don't feel right. What I recommend:

  • Get rid of the "Pick time" button and instead always display the time. Set the default time to 8:00 AM. Its an extra step to have to click "Pick time" to bring up the time selector.
  • Move the "Cancel" button to the left side.
  • Move the "Notify" button to where "Cancel" is right now and use the same styles as the "Cancel" button
  • For the days, can we add some margin between each day so they don't look so squashed together?

This would make the UI look more clean 🥇

simulator_screenshot_3348E94E-C5DC-4D3C-BE3A-EA569C8A47EA

@watadarkstar
Copy link
Collaborator

When I click "Pick time" the UI changes to this (which has a lot of white space and looks wonky):

simulator_screenshot_36C491D6-E588-4A0B-89DF-2642014ECD75

We don't need the "At 19:14" here as we already show the time below. Let's just always show the time selector and default it to 8 AM:

simulator_screenshot_DBFC2B0F-041B-45BA-BCA9-75AC8926BB05

@watadarkstar
Copy link
Collaborator

@SalikSayyed Aside from the UI, the code looks good and clean! I look forward to seeing the updated UI when you have a chance to tweak it :)

@watadarkstar
Copy link
Collaborator

@SalikSayyed Please also resolve the conflicts with master :)

@watadarkstar
Copy link
Collaborator

Hey @SalikSayyed can you give me write access to your fork so I can modify stuff if needed?

@SalikSayyed
Copy link
Author

image
image
image
@watadarkstar check this I updated with few addition, decreased the size entirely and Auto Cancel of dialog when set, no cancel wehn not set since it is dismissable I dont think cancel is needed.
As for TimePicker updated it for IOS, for android when clicking on the Timing displayed opens UI to put timing.
Please verify for IOS as well..

@SalikSayyed
Copy link
Author

SalikSayyed commented Sep 22, 2021

Hey @SalikSayyed can you give me write access to your fork so I can modify stuff if needed?

Yes,That would be great. I added u as collaborator sent invite. Or should I do something else for write access?? @watadarkstar

@watadarkstar
Copy link
Collaborator

@SalikSayyed Its looking better! Nice work. I will jump in and tweak stuff and then get this merged in once I tweak the UI a bit :)

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.

Local Push Notification Reminder
2 participants