London | 26 ITP January | Mouawia Elkhalifa | Sprint 3 | Alarm Clock#1087
London | 26 ITP January | Mouawia Elkhalifa | Sprint 3 | Alarm Clock#1087MouawiaElkhalifa wants to merge 3 commits intoCodeYourFuture:mainfrom
Conversation
MouawiaElkhalifa
added
the
Needs Review
MouawiaElkhalifa
force-pushed
the
sprint-3
branch
from
a6bbeae to
debc327
Compare
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.
Can you fix the issue? -
You missed updating
index.htmlaccording to an instruction inreadme.md.
cjyuan
added
Reviewed
MouawiaElkhalifa
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
(1) Currently when starting a new countdown, the application does not always return to a clean initial state, which can lead to inconsistent behavior between runs. (Hint: a user may not click the "Stop" button first before starting a new count down.)
You can consider introducing a dedicated reset function to return the app to a clean initial state to help ensure consistency.
(2) Can you make the "Stop" button to also stop the count down? You can add another "click" event listener to the button to avoid modifying the original code.
| const timeDisplay = document.getElementById("timeRemaining"); | ||
|
|
||
| if (isNaN(secondsLeft) || secondsLeft <= 0) { | ||
| timeDisplay.innerText = "Time Remaining: 00:00"; |
There was a problem hiding this comment.
Could consider calling updateDisplay(0) on line 8, trading few milliseconds for simpler code, and keep the logic of displaying time in one place.
There was a problem hiding this comment.
Thanks
I added a resetAlarm() function so every new countdown starts from a clean state.
I added another click listener to the Stop button so it also clears the active countdown.
I also moved the display formatting into updateDisplay() to keep the time rendering in one place.
cjyuan
removed
the
Needs Review
MouawiaElkhalifa
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Changes look good. Well done.
| if (countdown !== null) { | ||
| clearInterval(countdown); | ||
| countdown = null; | ||
| } |
There was a problem hiding this comment.
Could also consider using resetAlarm as the callback, trading slight inefficiency for simplicity (and to keep the logic of resetting the alarm in one place). The code won't break when pauseAlarm() is called twice.
cjyuan
added
Complete
|
Closing PR because the January ITP run has finished. Feel free to re-open if you're still working on it. |
3 participants
Learners, PR Template
Self checklist
Changelist
In this PR, I have completed the Alarm Clock assignment for Sprint 3.