Glasgow | 25-ITP-SEP | Shreef Ibrahim | Sprint 3| Alarm clock app #795
Conversation
shreefAhmedM
added
the
Needs Review
shreefAhmedM
changed the title
shreefAhmedM
changed the title
jennethydyrova
added
Review in progress
Sprint-3/alarmclock/alarmclock.js
Outdated
| function setAlarm() { | ||
| const timeRemaining = document.getElementById("timeRemaining"); | ||
| const alarmSet = document.getElementById("alarmSet").value; | ||
| alarmTime = parseInt(alarmSet); |
There was a problem hiding this comment.
You might want to add some value validation to ensure that you get integer and this line doesn't throw an error.
Sprint-3/alarmclock/alarmclock.js
Outdated
| } else { | ||
| clearInterval(countDown); | ||
| playAlarm(); | ||
| document.body.style.background = "#e14343"; |
There was a problem hiding this comment.
Background color is not set to red when alarmTime reaches 0. You might want to double check this logic
Sprint-3/alarmclock/alarmclock.js
Outdated
| "0" | ||
| )}:${String(seconds).padStart(2, "0")}`; | ||
| } | ||
| // DO NOT EDIT BELOW HERE |
There was a problem hiding this comment.
You are missing stopAlarm function implementation. Please double check the acceptance criteria for this PR.
jennethydyrova
added
Reviewed
shreefAhmedM
added
the
Needs Review
There was a problem hiding this comment.
hi @shreefAhmedM! Really great work! I have just two small comments and it's good to go.
Optionally, when you have time, I would like you to think how you could rewrite this cleaner without having many variables in a global scope. You can pass parameters to event listener functions or perhaps restructure your functions in a way that allows passing variables as parameters.
| }); | ||
| // set up alarm | ||
| function setAlarm() { | ||
| const val = parseInt(inputField.value); |
There was a problem hiding this comment.
What if inputField.value is empty? Can you try starting the alarm without entering anything into input field?
There was a problem hiding this comment.
When inputField.value is empty, the parseInt("") returns NaN,
So I have validated the condition if the value is NaN, Undefined , null or less than 0 , i used return it will stop any further execution so no nothing will happen. I can throw an error message instead.
There was a problem hiding this comment.
You mean you already have it in your code or you are going to push? I don't see validation against NaN, etc.
There was a problem hiding this comment.
i have already used it thing i did not push properly
|
|
||
| // stop or start flashing | ||
| let flashingInterval; | ||
| const flashingColors = ["#FF4136", "#2ECC40", "#0074D9", "#FFDC00"]; |
There was a problem hiding this comment.
It's better to keep variable local if it's not used elsewhere. Makes your code cleaner and reduces possibility of bugs due to accidental change to your variable.
jennethydyrova
removed
the
Needs Review
shreefAhmedM
added
the
Needs Review
|
Looks good to me, good job! |
jennethydyrova
added
Complete
2 participants
Self checklist
Here is a basic implementation of an alarm clock app using HTML and JavaScript