Birmingham | ITP-Jan | Roman Sanaye | Sprint 3 | Alarm-clock#1080
Birmingham | ITP-Jan | Roman Sanaye | Sprint 3 | Alarm-clock#1080RomanSanaye wants to merge 4 commits intoCodeYourFuture:mainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
RomanSanaye
added
the
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
bot
removed
the
Needs Review
RomanSanaye
changed the title
This comment has been minimized.
This comment has been minimized.
RomanSanaye
changed the title
RomanSanaye
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
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.
Consider introducing a dedicated reset function to return the app to a clean initial state to help ensure consistency.
Note: a user may not click the "Stop" button first before starting a new count down.
| let minutes = Math.floor(totalSeconds / 60); | ||
| let seconds = totalSeconds % 60; | ||
| timeRemaining.innerText = `Time Remaining: ${String(minutes).padStart(2, "0")}:${String(seconds).padStart(2, "0")}`; |
There was a problem hiding this comment.
This code is repeated on lines 36-38.
Consider refactoring the code into a function.
There was a problem hiding this comment.
Hi @cjyuan , Thank you for your feedback. I have added a resetAlarm() function that resets all state variables and UI, and now setAlarm() calls it first so the user doesn't need to press Stop before starting a new countdown.
cjyuan
added
Reviewed
|
|
||
| document.getElementById("stop").addEventListener("click", () => { | ||
| pauseAlarm(); | ||
| resetAlarm(); |
There was a problem hiding this comment.
Should you need to avoid modifying the code beyond the marker
// DO NOT EDIT BELOW HERE
you can use addEventListener() to add additional callbacks above the marker.
|
|
||
| function startTimer() { | ||
| alarmInterval = setInterval(() => { | ||
| if (totalSeconds > 0) { |
There was a problem hiding this comment.
Why check if totalSeconds > 0? Isn't it always true?
| stopAlarmBtn.style.display = "none"; | ||
| setAlarmBtn.style.display = "inline-block"; // ADD THIS - show set button |
There was a problem hiding this comment.
Could also consider
- using
visibility: hidden(suggestion: look up the difference betweendisplay: noneandvisibility: false - disable/enable the buttons by setting their
disabledproperty
(No change required)
RomanSanaye
added
the
Needs Review
|
|
||
| document.getElementById("stop").addEventListener("click", () => { | ||
| pauseAlarm(); | ||
| }); |
There was a problem hiding this comment.
You don't have to delete this. You can just perform resetAlarm() in the event listener you introduced on line 76.
cjyuan
added
Complete
2 participants
Learners, PR Template
Self checklist
Changelist
MM:SSformat.00:00and plays the alarm audio.Questions
No questions.