Glasgow| 25-ITP-May | Bassam Alshujaa | Sprint 3 | Data-Groups #703
Conversation
Bassam-Radwan
changed the title
Bassam-Radwan
changed the title
Bassam-Radwan
changed the title
Sprint-3/alarmclock/alarmclock.js
Outdated
| return; | ||
| } | ||
|
|
||
| setTimeout(playAlarm, seconds * 1000); // play alarm after the entered time |
There was a problem hiding this comment.
The alarm sound is playing correctly, but you also need to update the timer on the page.
| @@ -1,25 +1,68 @@ | |||
| function populateTodoList(todos) { | |||
| let list = document.getElementById("todo-list"); | |||
| // Write your code to create todo list elements with completed and delete buttons here, all todos should display inside the "todo-list" element. | |||
| list.innerHTML = ""; | |||
There was a problem hiding this comment.
Remember what the readme file asks for - you need a button that can toggle the completed state of the task. Without this, it's not possible to fully test this.
LonMcGregor
added
Reviewed
|
Thanks for your feedback, @LonMcGregor! I've updated the alarm clock to show the time remaining on screen and the todo list to delete tasks and add a line-through style to completed tasks. |
Bassam-Radwan
added
Needs Review
Sprint-3/alarmclock/alarmclock.js
Outdated
| timeOnScreen--; | ||
| document.getElementById( | ||
| "timeRemaining" | ||
| ).innerText = `TimeRemaining: ${mins}: ${timeOnScreen}`; |
There was a problem hiding this comment.
There is still a problem with the way you are handling displaying the timer. Here is a test case for you to try: Run the clock from 70 seconds. Can you see the problem that happens? Where in the code does the problem lie?
LonMcGregor
added
Reviewed
|
Thanks @LonMcGregor for your feedback. I've updated the alarm to work with different cases. |
Bassam-Radwan
added
Needs Review
|
Good changes. It now handles larger inputs right. But what happens if I start a clock with a long timer (e.g. 60 seconds), then while it is running I start the timer again with a short time, e.g. 2 seconds? What would you expect to happen? |
LonMcGregor
added
Reviewed
|
I updated the function by adding a clearInterval at the begining of the set alarm. I think now it works well. |
Bassam-Radwan
added
Needs Review
|
Great. You are done with this sprint now. Well done! |
LonMcGregor
added
the
Complete
Learners, PR Template
Self checklist
Changelist
I've implemented all files and added changes to the Html files
Questions
Ask any questions you have for your reviewer.