NW| 25-ITP-Sep | Ahmad Hmedan | Sprint 3 | Alarm clock #911
Conversation
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
AhmadHmedann
added
the
Needs Review
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
github-actions
bot
removed
the
Needs Review
AhmadHmedann
added
the
Needs Review
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
github-actions
bot
removed
the
Needs Review
AhmadHmedann
added
the
Needs Review
jaymes15
added
Review in progress
| @@ -1,4 +1,41 @@ | |||
| function setAlarm() {} | |||
| let timerId = null; | |||
| let timerId = null; | ||
| let remainingSeconds = 0; | ||
|
|
||
| function updateDisplay() { |
Sprint-3/alarmclock/alarmclock.js
Outdated
|
|
||
| function updateDisplay() { | ||
| let minutes = String(Math.floor(remainingSeconds / 60)).padStart(2, "0"); | ||
| let seconds = String(remainingSeconds % 60).padStart(2, "0"); |
There was a problem hiding this comment.
Use const instead of let for minutes and seconds because they are never reassigned.
Sprint-3/alarmclock/alarmclock.js
Outdated
| let remainingSeconds = 0; | ||
|
|
||
| function updateDisplay() { | ||
| let minutes = String(Math.floor(remainingSeconds / 60)).padStart(2, "0"); |
There was a problem hiding this comment.
Proper time formatting with padStart()
Nice One!
| ).textContent = `Time Remaining: ${minutes}:${seconds}`; | ||
| } | ||
|
|
||
| function startCountdown() { |
| function startCountdown() { | ||
| if (remainingSeconds < 0) remainingSeconds = 0; | ||
| timerId = setInterval(() => { | ||
| updateDisplay(); |
There was a problem hiding this comment.
This runs before decrementing remainingSeconds, causing the countdown to visually lag by one second.
| timerId = setInterval(() => { | ||
| updateDisplay(); | ||
|
|
||
| if (remainingSeconds <= 0) { |
There was a problem hiding this comment.
The sequence of logic in this if block would cause some issues.
This is the order you are following currently:
- The timer checks for zero before subtracting.
- So the alarm plays one second late.
- Also remainingSeconds becomes -1 often.
Why not Subtract first, then check?
| } | ||
|
|
||
| function setAlarm() { | ||
| const input = document.getElementById("alarmSet"); |
There was a problem hiding this comment.
I think we can have better validation in setAlarm function.
- If input is negative
- if input is a string of text
- you did a good job checking for empty string
There was a problem hiding this comment.
Thank you for your feedback — it was really helpful, and I understood what you meant straight away.
In the HTML file, Their is a validation rule using the type attribute in Index.html file. Should I always treat HTML and JavaScript as separate layers and include validation in both?
|
@AhmadHmedann It’s totally fine that it took you 8 hours instead of the suggested 4. As you keep coding and solving more problems, you’ll naturally get faster and more confident. Progress comes with practice, so don’t worry about the extra time — it’s part of the learning process. |
jaymes15
added
Reviewed
…remainSecond before calling updateDisplay \n handle zero and negative-input
AhmadHmedann
added
Needs Review
2 participants
Learners, PR Template
Self checklist
Changelist
I have change the title in index.hml and update the code in alarmclock.js
Questions
I know it’s a silly question. They said this task should take 4 hours, but it took me at least 8. Is that normal? I also looked at some completed PRs to get a few clues.