London | ITP-SEP25 | Imran Mohamed | Sprint 3 | Alarm Clock #822
Conversation
i786m
added
the
Needs Review
illicitonion
added
Review in progress
illicitonion
left a comment
illicitonion
left a comment
There was a problem hiding this comment.
This is generally looking good, but there's a slight expectations mismatch. I expect when I press the "start timer" button, for it to instantly show the time I'm counting down from. But yours waits a second before it does so. That's probably also why your tests are getting off-by-one-second issues. Can you think how to give this effect?
Sprint-3/alarmclock/alarmclock.js
Outdated
| timeRemaining.textContent = `Time Remaining: ${String( | ||
| minutes | ||
| ).padStart(2, "0")}:${String(seconds).padStart(2, "0")}`; |
There was a problem hiding this comment.
This works, but it's quite hard to read what this line of code is doing. Generally any time a string interpolation runs across multiple lines, it's quite hard to read. Can you think how to make this easier to follow at a glance?
There was a problem hiding this comment.
I have extracted the updating of the time remaining to its own function and made some changes to better increase readability.
illicitonion
added
Reviewed
…wn to attempt pause functionality- still unfinished
i786m
added
Needs Review
|
I have added a countdown.js file in order to attempt the pause button of the stretch tasks but I was running into some issues, which I will come back to in the future. If possible, please have a look and see if there's anything glaringly obvious that I am missing. |
I tried running it locally (after adding a |
illicitonion
left a comment
illicitonion
left a comment
There was a problem hiding this comment.
Looking good, well done!
| const alarmTimeInSeconds = alarmTimeInput.value; | ||
| let totalSeconds = parseInt(alarmTimeInSeconds); |
There was a problem hiding this comment.
These are much clearer names, but now I'm not sure the difference between alarmTimeInSeconds and totalSeconds - can you differentiate these better too? :)
Sprint-3/alarmclock/alarmclock.js
Outdated
| timeRemaining.textContent = `Time Remaining: ${String( | ||
| minutes | ||
| ).padStart(2, "0")}:${String(seconds).padStart(2, "0")}`; |
illicitonion
added
Complete
2 participants
Learners, PR Template
Self checklist
Changelist
Implemented the countdown timer and flashing background
Questions
I am having some issues with getting the tests to pass have made changes to code which makes a previously passed test fail. Please advise on changes I can make. Is there anything in particular I should take into consideration when ensuring I'm meeting tdd requirements when my outputs and expected outputs are a second off either way?