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 2 commits into
Conversation
Dagim-Daniel
added
Needs Review
cjyuan
added
Reviewed
Dagim-Daniel
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Most of the changes you made are good.
Sorry that I just noticed some more stuffs you could also improve.
| setButton.disabled = false; | ||
| document.body.style.background = ""; | ||
| } | ||
| let countdownInterval; |
There was a problem hiding this comment.
Between this variable and the variable declared on line 6, one of them is not used and one of the them seems to be misspelled.
| let timeleft = Number(totalSeconds.value); | ||
|
|
||
| if (timeleft <= 0 || isNaN(timeleft)) { | ||
| alert("please enter a number greater than zero (0)!"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
What type of number should the input be?
| display.textContent = formatTime(timeleft); | ||
|
|
||
| const warningTime = 10; // seconds | ||
| clearInterval(conuntdownInterval); |
There was a problem hiding this comment.
Is clearing countdownInterval necessary before starting a new countdown?
If clearing it is necessary, then it is likely that we should also call reset() here.
| stopButton.addEventListener("click", () => { | ||
| clearInterval(conuntdownInterval); | ||
| reset(); | ||
| pauseAlarm(); | ||
| }); |
There was a problem hiding this comment.
addEventListener() can add multiple event listeners (callbacks) to a DOM object.
Normally if we could modify all the code, we could just update the original callback (lines 73-75). Since we are adding another onclick callback to stopButton, we should avoid repeating the task performed by the original callback.
| display.textContent = formatTime(timeleft); | ||
|
|
||
| const warningTime = 10; // seconds | ||
| clearInterval(conuntdownInterval); | ||
|
|
||
| conuntdownInterval = setInterval(() => { | ||
| timeleft--; |
There was a problem hiding this comment.
New countdown implementation is good. You could consider moving the statement that displays the initial time on line 34 to just before line 39 so that the code in the function reads more like:
// Process input
// Reset/initialize states
// Start countdown
2 participants
Learners, PR Template
Self checklist
Changelist
tasks under Sprint 3 alarm has been done