-
-
Notifications
You must be signed in to change notification settings - Fork 193
Glasgow | 25-ITP-Sep | Mohammed Abdoon | Sprint 3| Alarm Clock #848
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Your PR couldn't be matched to an assignment in this module. Please check its title is in the correct format, and that you only have one PR per assignment. 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). |
1 similar comment
|
Your PR couldn't be matched to an assignment in this module. Please check its title is in the correct format, and that you only have one PR per assignment. 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). |
AbogeJr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall nice work @M-Abdoon. I've highlighted a few improvements you could make in the comments 👍
Sprint-3/alarmclock/alarmclock.js
Outdated
| }, 1000); | ||
| } | ||
|
|
||
| function toMMSS (seconds) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function could have a more descriptive name? Maybe something that easily describes what the function does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. I changed the name to 'formatTimeMMSS()' so it shows what it actually does.
Sprint-3/alarmclock/alarmclock.js
Outdated
| timeRemainingHTML.innerHTML = `Time Remaining: ${toMMSS(secondsLeft)}`; | ||
| pageTitle.innerHTML = `Time Remaining: ${toMMSS(secondsLeft)}`; | ||
|
|
||
| document.getElementById("pause").addEventListener("click", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is the best place to initialize an event listener. Please check it out to see why it would be problematic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be problematic because it keeps adding a new listener every 100 ms. I moved the event out of setInterval().
Sprint-3/alarmclock/alarmclock.js
Outdated
| let secondsLeft = Math.max(0, Math.ceil(timeDifference / 1000)); | ||
|
|
||
| timeRemainingHTML.innerHTML = `Time Remaining: ${toMMSS(secondsLeft)}`; | ||
| pageTitle.innerHTML = `Time Remaining: ${toMMSS(secondsLeft)}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a better way to change the title of a html document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I now used this way:
document.title =
instead of:
pageTitle.innerHTML =
Sprint-3/alarmclock/alarmclock.js
Outdated
| function setAlarm() {} | ||
| function setAlarm() { | ||
| const timer = Number(document.querySelector("#alarmSet").value); | ||
| const timeRemainingHTML = document.querySelector("#timeRemaining"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can have a better name for the variable here since we are referencing a single element?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to ' timeRemainingEl ' which means ' time remaining element '. I think that's a better way to name it.
Sprint-3/alarmclock/alarmclock.js
Outdated
| const pageTitle = document.querySelector("title"); | ||
|
|
||
| let targetTime = new Date().getTime() + timer * 1000; | ||
| console.log(targetTime); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine during development but typically we clean them up before submission of work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exactly! I removed it.
Sprint-3/alarmclock/alarmclock.js
Outdated
| const doAlarm = setInterval( () => { | ||
| let timeThisSecond = new Date().getTime(); | ||
| let timeDifference = targetTime - timeThisSecond; | ||
| let secondsLeft = Math.max(0, Math.ceil(timeDifference / 1000)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job using the Math functions here. 👌
Although, a minor improvement you should look into is when to use let to initialize variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you ^^.
I replaced 'let' with 'const' since we won't need to change their values.
Thank you so much for your review. |
AbogeJr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there 👌
| document.getElementById("pause").addEventListener("click", () => { | ||
| pauseAlarm(); | ||
| clearInterval(doAlarm); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite. We're not supposed to edit the code in this section. Please check again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I changed its place.
Sprint-3/alarmclock/alarmclock.js
Outdated
| const secondsLeft = Math.max(0, Math.ceil(timeDifference / 1000)); | ||
|
|
||
| timeRemainingEl.innerHTML = `Time Remaining: ${formatTimeMMSS(secondsLeft)}`; | ||
| document.title = `Time Remaining: ${formatTimeMMSS(secondsLeft)}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there but please check the first instruction about the "title" on the alarmclock readme.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yes. I should have changed the page title so I put a default one and then it changes when the user clicks on the button.
I have set the title.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I see the instruction is a little ambiguous, could be worded better but generally you won't want to update the page title, at least not in this way (every second).
![]()
ref:
First off, once you've branched off main, then update the title element in index.html to "Alarm clock app" You will need to write your implementation in alarmclock.js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it maybe be like " Counting down ... " once the user clicks on the button? or maybe it should not change at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The title (on the browser tab) should remain as it is. The title (h1 inside the page should update)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I just updated the code, now the page title on the browser tab does not change anymore.
|
Thank you so much. |
AbogeJr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. One last thing, please check the <title> in the alarmclock.html
|
I changed the title. Thank you. |
Learners, PR Template
Self checklist
Changelist
Alarm clock app exercise done.
Questions
No questions so far, thank you.