London | May-26 | Liridona Shehu | Sprint 3 | Module-Data-Groups - Alarmclock - #1309
London | May-26 | Liridona Shehu | Sprint 3 | Module-Data-Groups - Alarmclock#1309shehu-dona wants to merge 4 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
shehu-dona
changed the title
shehu-dona
added
📅 Sprint 3
| let currentMinutes = Math.floor(remainingSeconds / 60) | ||
| .toString() | ||
| .padStart(2, "0"); | ||
| let currentSeconds = Math.floor(remainingSeconds % 60) | ||
| .toString() | ||
| .padStart(2, "0"); | ||
| timeRemainingHeading.innerText = `Time Remaining: ${currentMinutes}:${currentSeconds}`; |
There was a problem hiding this comment.
This code is duplicated from line 9 to 16. How can you remove the duplication? (Duplicated code makes maintenance harder as you would have make changes in multiple places. Imagine the formatting should change for example)
There was a problem hiding this comment.
Created a function formatTime() and call it when i need it.
| clearInterval(intervalId); | ||
|
|
||
| const initialSeconds = Number(document.getElementById("alarmSet").value); | ||
| remainingSeconds = initialSeconds; |
There was a problem hiding this comment.
What happens if I start the alarm with 0 seconds or a negative value?
There was a problem hiding this comment.
Checked for 0, negative or empty input.
Luro91
added
Review in progress
shehu-dona
added
the
Needs Review
|
I don't see any pushed changes since my comments. Please double check. You can always use the files changes view in Github or commit view to see if your changes have been pushed. |
Luro91
added
Reviewed
shehu-dona
added
the
Needs Review
|
Hello @Luro91, |
| if (!initialSeconds || initialSeconds <= 0) { | ||
| timeRemainingHeading.innerText = "Please enter a positive number of seconds"; | ||
| return; | ||
| } |
There was a problem hiding this comment.
This code is not matching the standard for formatting. (Indendation inside the brackets). How can you ensure consistent formatting of all your code?
Luro91
removed
the
Needs Review
shehu-dona
added
the
Needs Review
Luro91
left a comment
Luro91
left a comment
There was a problem hiding this comment.
Well done. The code is working and well structured now.
As a tip: you can configure the Prettier extension to always format the code when you save the file. This way you don't have to think about it.
Luro91
added
Complete
|
Thank you for reviewing my PR @Luro91. I do have prettier installed but in some projects it is not working sometimes.I find out that i have to run npm install. |
3 participants
Self checklist
Changelist
A PR with a alarm app