West Midlands | ITP-Sept-25 | Georgina Rogers | Sprint 3 | Alarm Clock #905
Conversation
|
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Wrong number of parts separated by |s 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. |
zilinskyte
changed the title
|
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. |
3 similar comments
|
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. |
|
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. |
|
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. |
zilinskyte
added
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Code is very solid.
At "some point", the clock could appear as -1:-1. Is that what you intended?
| window.incrementTime = incrementTime; | ||
| window.setAlarm = setAlarm; | ||
| window.stopTimer = stopTimer; |
There was a problem hiding this comment.
Are you aware of some of the pros and cons between these two approaches of invoking a callback function?
(1) The approach you are using
(2) Assign an ID to the element and attach a callback function to the element as
document.getElementbyId('id-of-the-element').addEventListener('event-name', callback);
| totalSeconds = parsed; | ||
| updateDisplay(); | ||
|
|
||
| if (timer) clearInterval(timer); |
There was a problem hiding this comment.
Can you think of anything else that should also be reset before starting a new countdown?
cjyuan
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist
Alarm clock function written, HTML improved for readability, and alarm tested in live server
Questions
Had to copy over all my changes from a another closed pull request due to conflicting changes.