-
Notifications
You must be signed in to change notification settings - Fork 1
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
Alarm component #77
Alarm component #77
Conversation
} | ||
}; | ||
|
||
convertSeconds = (seconds) => { |
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.
Would likely want to make these into utility functions that can be used throughout app. I think I would maybe consider that we start using "momentjs" for time and date manipulations and stuff, but maybe a bit overkill too
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.
You are right. This should be extracted into a service class to deal with time related conversions and calculations. However, I wouldnt add a dependency just for this.
Creating the service and refactoring will be another task. Here
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.
Very nice! I made it so that the border for the container also reflects the color of the background. Also flashing yellow and red is what the client wanted so I made that change as well. Nice work!
As for extracting the methods into a service, there is an issue linked below.
Also for enhancements, we should probably add vibrate and ringtones. I will make an issue for it here
#60