London | 26-ITP-May | Damilola Odumosu| Sprint 3 | Alarm Clock - #1360
London | 26-ITP-May | Damilola Odumosu| Sprint 3 | Alarm Clock#1360d-odumosu wants to merge 1 commit into
Conversation
d-odumosu
added
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
-
Code works fine if a user only clicks the "Set Alarm" button once. However, if the user enters a time and then clicks the "Set Alarm" button multiple times, the countdown clock will not display properly. Could you fix the issue?
-
Currently when starting a new countdown, the application does not always return to a clean initial state, which can lead to inconsistent behaviour between runs.
Note: a user may not click the "Stop" button first before starting a new count down.
| @@ -1,4 +1,28 @@ | |||
| function setAlarm() {} | |||
| const input = document.getElementById("alarmSet"); | |||
| const timeDisplay = document.querySelector("span"); | |||
There was a problem hiding this comment.
span is a fairly generic element, so using it as the selector for the time display can make the code less specific and more fragile. Could you find out why selecting a span for this purpose is not considered good practice, and update the selector accordingly?
| let remainingMinutes = Math.floor(timeInput / 60); | ||
| let remainingSeconds = timeInput % 60; | ||
| remainingMinutes = remainingMinutes.toString().padStart(2, "0"); | ||
| remainingSeconds = remainingSeconds.toString().padStart(2, "0"); | ||
| timeDisplay.textContent = `${remainingMinutes}:${remainingSeconds}`; |
There was a problem hiding this comment.
Code on lines 15-19 is very similar to those on lines 6-11.
To adhere to the DRY principle, could you refactor the repeated code into a reusable function?
| const timeDisplay = document.querySelector("span"); | ||
|
|
||
| function setAlarm() { | ||
| let timeInput = Number(input.value); |
There was a problem hiding this comment.
Some input values could make your app behave abnormally. Could you add code to sanitise or reject them?
cjyuan
added
Reviewed
2 participants
Learners, PR Template
Self checklist