Sheffield | 26-ITP-Jan | Mahmoud Shaabo | Sprint 3 | Alarm Clock App#1155
Sheffield | 26-ITP-Jan | Mahmoud Shaabo | Sprint 3 | Alarm Clock App#1155mahmoudshaabo1984 wants to merge 2 commits intoCodeYourFuture:mainfrom
Conversation
|
Hi @cifarquhar, Here is my Pull Request for the Alarm Clock App. I successfully implemented the timer logic using Looking forward to your feedback! |
cjyuan
left a comment
There was a problem hiding this comment.
-
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.
-
When input is 1, the alarm is played one second later -- an expected behavior.
However, when input is 0, the alarm is also played one second later (instead of being played immediately). Can you make the app's behaviour more consistent?
Sprint-3/alarmclock/alarmclock.js
Outdated
| const mins = String(Math.floor(totalSeconds / 60)).padStart(2, "0"); | ||
| const secs = String(totalSeconds % 60).padStart(2, "0"); | ||
| document.getElementById("timeRemaining").innerText = | ||
| "Time Remaining: " + mins + ":" + secs; |
There was a problem hiding this comment.
Instead of keeping several copies of the code for displaying time (lines 18-21, lines 44-47, lines 32-33),
it is better to define a function to update the time display and call the function to update the time display.
Advantages:
- Keep the logic for updating time display in one place
- Less code
|
Hi CJ, Thank you for the excellent feedback! I have just pushed an update to address all your points:
All Jest tests are still passing successfully. Please let me know if everything looks good to go! Best regards, |
cjyuan
left a comment
There was a problem hiding this comment.
Previous changes are good.
I just have two more suggestions for you to consider.
| document.getElementById("timeRemaining").innerText = | ||
| "Please enter a valid number of seconds (0 or above)."; |
There was a problem hiding this comment.
It is better to introduce a separate element to show feedback message, and leave "#timeRemaining" for only displaying time.
| updateTimeDisplay(0); | ||
|
|
||
| // Change background colour to signal the alarm | ||
| document.body.style.backgroundColor = "red"; |
There was a problem hiding this comment.
To better separate presentation logic from application logic, you can consider defining a CSS class, and use classList.toggle() to apply/remove the style. For example,
document.body.classList.toggle("alarm-activated", true); // apply style
document.body.classList.toggle("alarm-activated", false); // remove style
Hello Reviewers / CodeYourFuture Team,
I have completed the Alarm Clock App project for Sprint 3.
Acceptance Criteria Met:
index.htmlto "Alarm clock app".setAlarm()inalarmclock.jsto read the input and display the time inmm:ssformat.setInterval()to decrement the timer every 1 second.00:00, the interval is cleared, the background changes to red, and the alarm audio plays.Thank you for your review!