Skip to content

Conversation

@Mahtem
Copy link

@Mahtem Mahtem commented Dec 6, 2025

Learners, PR Template

Self checklist

  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • My changes meet the requirements of the task
  • I have tested my changes
  • My changes follow the style guide

Changelist

Alarm clock, implemented its index.html modified and styles added.

Questions

Question will be forwarded to slack.

@Mahtem Mahtem added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. 📅 Sprint 3 Assigned during Sprint 3 of this module labels Dec 6, 2025
Copy link
Contributor

@cjyuan cjyuan left a 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?

Comment on lines 79 to 92
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";
}
Copy link
Contributor

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.

Copy link
Author

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";
}

Comment on lines 16 to 18
if (countdownInterval) clearInterval(countdownInterval);
stopFlashing();
paused = false;
Copy link
Contributor

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).

Copy link
Author

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.


function pauseAlarm() {
paused = true;
if (audio) audio.pause();
Copy link
Contributor

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)?

Copy link
Author

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.

</div>
</div>

<script src="alarmclock.js"></script>
Copy link
Contributor

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.

Copy link
Author

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.

script has been updated in the index.html as :

<script src="alarmclock.js" type="module"></script>

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Dec 9, 2025
@Mahtem
Copy link
Author

Mahtem commented Dec 10, 2025

Regardless of whether the input is 0 or 1, the alarm sounds 1 second later. Can you address this inconsistency?

Thank you @cjyuan , for pointing that out, now the alarm has been made to sound for >=1 sec.

@Mahtem Mahtem added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Dec 11, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Dec 12, 2025

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.
https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax

@cjyuan cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Reviewed Volunteer to add when completing a review with trainee action still to take. labels Dec 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed. 📅 Sprint 3 Assigned during Sprint 3 of this module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants