-
-
Notifications
You must be signed in to change notification settings - Fork 193
Manchester | 25-ITP-Sep | Mahtem T. Mengstu | Sprint 3 | Alarmclock #916
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
cjyuan
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.
Regardless of whether the input is 0 or 1, the alarm sounds 1 second later. Can you address this inconsistency?
| function startFlashing() { | ||
| const body = document.body; | ||
| let isBlue = false; | ||
| flashInterval = setInterval(() => { | ||
| body.style.backgroundColor = isBlue ? "white" : "#add8e6"; | ||
| isBlue = !isBlue; | ||
| }, 500); | ||
| } | ||
|
|
||
| function stopFlashing() { | ||
| clearInterval(flashInterval); | ||
| flashInterval = null; | ||
| document.body.style.backgroundColor = "white"; | ||
| } |
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.
Could consider implementing the "flashing background" effect using only CSS, and then add/remove a class to start/stop flashing.
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.
Could consider implementing the "flashing background" effect using only CSS, and then add/remove a class to start/stop flashing.
Thank you @cjyuan, it has been implemented using only css.
function startFlashing() {
document.body.classList.add("alarm-flashing");
}
function stopFlashing() {
document.body.classList.remove("alarm-flashing");
document.body.style.backgroundColor = "white";
}
Sprint-3/alarmclock/alarmclock.js
Outdated
| if (countdownInterval) clearInterval(countdownInterval); | ||
| stopFlashing(); | ||
| paused = false; |
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.
Could also consider calling resetAlarm() (even if it performs something extra).
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.
Could also consider calling
resetAlarm()(even if it performs something extra).
It has been made to reset everything completely.
Sprint-3/alarmclock/alarmclock.js
Outdated
|
|
||
| function pauseAlarm() { | ||
| paused = true; | ||
| if (audio) audio.pause(); |
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.
Why not just write audio.pause() (without the if statement)?
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.
Why not just write
audio.pause()(without the if statement)?
Yes, it's correct... calling "audio.pause()" alone is enough and works normally.
Sprint-3/alarmclock/index.html
Outdated
| </div> | ||
| </div> | ||
|
|
||
| <script src="alarmclock.js"></script> |
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.
Suggestion: Consider loading script.js as an ES module to isolate its scope from the global context as:
<script src="script.js" type="module"></script>
This ensures that variables, functions, and imports in script.js don't leak into the global namespace,
helps prevent naming conflicts, and enables the use of modern JavaScript features like import and export.
Note: With type="module", defer is automatically enforced.
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.
Suggestion: Consider loading
script.jsas an ES module to isolate its scope from the global context as:
<script src="script.js" type="module"></script>This ensures that variables, functions, and imports in script.js don't leak into the global namespace, helps prevent naming conflicts, and enables the use of modern JavaScript features like import and export.
Note: With
type="module",deferis automatically enforced.
script has been updated in the index.html as :
<script src="alarmclock.js" type="module"></script>
Thank you @cjyuan , for pointing that out, now the alarm has been made to sound for >=1 sec. |
|
Changes look great. Excellent work. Suggestion: Check out what Markdown syntax is supported by GitHub (especially for embedding code and code block). You will likely need them in the future. |
Learners, PR Template
Self checklist
Changelist
Alarm clock, implemented its index.html modified and styles added.
Questions
Question will be forwarded to slack.