London | 26-ITP-May | Dagim Daniel | Sprint 3 | Alarm clock - #1331
London | 26-ITP-May | Dagim Daniel | Sprint 3 | Alarm clock#1331Dagim-Daniel wants to merge 1 commit into
Conversation
Dagim-Daniel
added
Needs Review
|
|
||
| // button event handler | ||
| setButton.addEventListener("click", () => { | ||
| let timeleft = Number(totalSeconds.value); |
There was a problem hiding this comment.
Some unusual input values can make your app behave abnormally. Can you add code to sanitise them?
| conuntdownInterval = setInterval(() => { | ||
| if (timeleft == 0) { | ||
| clearInterval(conuntdownInterval); | ||
| document.body.style.backgroundColor = "grey"; | ||
| playAlarm(); | ||
| } | ||
|
|
||
| if (timeleft <= 10) { | ||
| display.style.color = "red"; | ||
| } | ||
|
|
||
| if (timeleft >= 0) { | ||
| display.textContent = formatTime(timeleft); | ||
| timeleft--; | ||
| } | ||
| }, 1000); |
There was a problem hiding this comment.
When a user clicks the "Set" button with 1 second as input, the user will only see "00:00" 2 seconds later. Is the extra 1 second delay at the beginning by design?
| if (timeleft <= 10) { | ||
| display.style.color = "red"; | ||
| } |
There was a problem hiding this comment.
What does this 10 represent? Numbers other than 0 and 1 are generally considered magic numbers, so it is good practice to replace them with named constants.
| setButton.disabled = true; | ||
|
|
||
| // time 0 | ||
|
|
||
| if (timeleft <= 0) { | ||
| alert("please enter a number greater than 0!"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Would it be better to disable the button only when a countdown is successfully set?
| function clearEverything() { | ||
| display.style.color = "black"; | ||
| setButton.disabled = false; | ||
| document.body.style.background = ""; | ||
| } |
There was a problem hiding this comment.
What about calling it "reset..."?
Could also consider resetting the display time to "00:00" here.
cjyuan
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist
tasks under Sprint 3 alarm has been done