ITP JAN 2025/ELFREDAH KEVIN-ALERECHI/DAT GROUP/ALARM CLOCK #494
Conversation
Elfredah
changed the title
cjyuan
added
the
Review in progress
There was a problem hiding this comment.
The alarm sound cannot play but the code is working (to certain extent); the "Stop Button" is behaving mysteriously.
The branch should only only modified files that belong to the Alarm Clock project. Since you commit ALL changes in one commit instead of committing one file at a time, you can consider fixing the branch in the following way:
- Replace all files (in the current branch) that do not belong to the Alarm Clock project by the same files in the
mainbranch. You can download yourmainbranch as a ZIP file from your Github repo.
There was a problem hiding this comment.
It is a better practice to separate JS code from HTML code. Can you transfer the JS code to alarmclock.js?
| <input type="text" id="alarm-time" placeholder="Enter time in seconds"> | ||
| <button onclick="setAlarm()">Set Alarm</button> | ||
| <button id="stop-alarm" onclick="stopAlarm()" style="display: none;">Stop Alarm</button> | ||
| <audio id="alarm-sound" src="https://www.soundjay.com/button/beep-07.wav" loop></audio> |
There was a problem hiding this comment.
This audio cannot be loaded. Why not use the one in the project folder?
| function formatTime(seconds) { | ||
| let minutes = Math.floor(seconds / 60); | ||
| let remainingSeconds = seconds % 60; | ||
| return `${minutes < 10 ? '0' + minutes : minutes}:${remainingSeconds < 10 ? '0' + remainingSeconds : remainingSeconds}`; |
|
|
||
| timeRemaining = alarmTime; | ||
| timeRemainingTitle.innerText = formatTime(timeRemaining); | ||
| stopButton.style.display = 'inline'; |
There was a problem hiding this comment.
You can also consider using visibility: visible and visibility: hidden to show/hide an element.
Using display : inline and display : none to show/hide an element can cause the elements in the UI rearranged.
| function stopAlarm() { | ||
| alarmSound.pause(); | ||
| alarmSound.currentTime = 0; | ||
| document.body.style.backgroundColor = ''; // Reset background color | ||
| stopButton.style.display = 'none'; // Hide stop button | ||
| } |
There was a problem hiding this comment.
If a user clicks the "Stop Button" while the clock is still counting down, the "Stop Button" will "disappear" (and reappear when the clock reaches 0). Is this a bug of feature?
cjyuan
added
👀 Review Git
|
On separate note, it is also a good practice to include a brief description of a PR in the PR template: |
Elfredah
added
🎯 Topic Code Review
|
I don't see anything changed since I last reviewed this PR. Have you pushed your changes to Github? |
cjyuan
added
Reviewed
Done all requirmeents
<!
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.