Skip to content

LONDON | MAY 2025 | JESUS DEL MORAL | SPRINT3 | ALARM CLOCK#671

Closed
delmorallopez wants to merge 9 commits intoCodeYourFuture:mainfrom
delmorallopez:Sprint-3
Closed

LONDON | MAY 2025 | JESUS DEL MORAL | SPRINT3 | ALARM CLOCK#671
delmorallopez wants to merge 9 commits intoCodeYourFuture:mainfrom
delmorallopez:Sprint-3

Conversation

@delmorallopez
Copy link
Copy Markdown

PR of AlarmClock exercise, include 3 files:

  • alarmclock.js
  • alarmclock.test.js
  • alarmclockapp.html

I setup the option to change the colour background when the timer reach zero

@delmorallopez delmorallopez added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 21, 2025
@illicitonion illicitonion changed the title LONDON | MAY 2025 | JESUS DEL MORAL | SPRINT3 | ALARMCLOCK LONDON | MAY 2025 | JESUS DEL MORAL | SPRINT3 | ALARM CLOCK Jul 22, 2025
@delmorallopez delmorallopez changed the title LONDON | MAY 2025 | JESUS DEL MORAL | SPRINT3 | ALARM CLOCK LONDON | MAY 2025 | JESUS DEL MORAL | SPRINT3 | ALARM CLOCK-QUOTE GENERATOR Jul 22, 2025
Copy link
Copy Markdown
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.

  • After the user clicks the "Set Alarm" button, there is a one second delay before the clock starts counting down. For example, if the input is 1, then the alarm will only sound 2 seconds after the user clicks the "Set Alarm" button. Can you address this issue?

  • Branch is not clean. Can you revert the change made in the quote-generator folder?

Comment thread Sprint-3/alarmclock/alarmclock.js Outdated
Comment on lines +1 to +3
timeSetup = document.querySelector("#alarmSet"); // Access to alarmset

timeCountdown = document.querySelector("#timeRemaining"); // Access to timeRemaining
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did you first declare these two variables?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I declared at the begining, before setAlarm function()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 1 and 3 are not "variable declaration"; they are merely two assignment statements.
Line 5 is a variable declaration.

Comment thread Sprint-3/alarmclock/alarmclock.js Outdated
Comment thread Sprint-3/alarmclock/alarmclock.js Outdated
Comment on lines +9 to +12
const minutes = String(Math.floor(totalSeconds / 60)).padStart(2, "0");
const seconds = String(totalSeconds % 60).padStart(2, "0");

timeCountdown.innerText = `Time Remaining: ${minutes}:${seconds}`;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If input is an invalid number, we should avoid changing the clock display (and then immediately replace the clock display by another text (at line 24).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 24 has been modified, if is an invalid number, the display clock won't change

if (isNaN(totalSeconds) || totalSeconds <= 0) {
timeCountdown.innerText = "Time Remaining: 00:00"; // Return if the input is not a number or less than or equal to 0
return;
}

Comment thread Sprint-3/alarmclock/alarmclock.js Outdated
let countdownInterval; // Variable to setup to 0 the countdown

function setAlarm() {
const totalSeconds = parseInt(timeSetup.value, 10);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without sanitizing the input, the display could get corrupted.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I validate the input

const rawInput = timeSetup.value.trim(); // Remove surrounding spaces
const totalSeconds = parseInt(rawInput, 10);

// Validate the input
if (isNaN(totalSeconds) || totalSeconds < 0) {
timeCountdown.innerText = "Please enter a valid number of seconds.";
document.body.style.backgroundColor = "";
return;
}

@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 Jul 26, 2025
@delmorallopez delmorallopez changed the title LONDON | MAY 2025 | JESUS DEL MORAL | SPRINT3 | ALARM CLOCK-QUOTE GENERATOR LONDON | MAY 2025 | JESUS DEL MORAL | SPRINT3 | ALARM CLOCK Jul 27, 2025
@delmorallopez delmorallopez added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 27, 2025
Copy link
Copy Markdown
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.

These two issues still have not yet been addressed:

  • After the user clicks the "Set Alarm" button, there is a one second delay before the clock starts counting down. For example, if the input is 1, then the alarm will only sound 2 seconds after the user clicks the "Set Alarm" button. Can you address this issue?

  • Branch is not clean. Can you revert the change made in the quote-generator folder?

Comment thread Sprint-3/alarmclock/alarmclock.js Outdated
Comment on lines +1 to +3
timeSetup = document.querySelector("#alarmSet"); // Access to alarmset

timeCountdown = document.querySelector("#timeRemaining"); // Access to timeRemaining
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 1 and 3 are not "variable declaration"; they are merely two assignment statements.
Line 5 is a variable declaration.

Comment thread Sprint-3/alarmclock/alarmclock.js Outdated
Comment on lines +19 to +22
const minutes = String(Math.floor(totalSeconds / 60)).padStart(2, "0");
const seconds = String(totalSeconds % 60).padStart(2, "0");

timeCountdown.innerText = `Time Remaining: ${minutes}:${seconds}`;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you define updateDisplay() with a parameter (and define it in the outer scope), you could replace these three lines of code with a function call.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I called the function updateDisplay() passing the parameter -- updateDisplay(totalSeconds);
I declarated the variable total second in the outer scope, its working fine

Comment thread Sprint-3/alarmclock/alarmclock.js Outdated
Comment on lines +39 to +60
const updateDisplay = () => {
const minutes = String(Math.floor(totalSeconds / 60)).padStart(2, "0");
const seconds = String(totalSeconds % 60).padStart(2, "0");
timeCountdown.innerText = `Time Remaining: ${minutes}:${seconds}`;
};

updateDisplay(); // Show first tick instantly

countdownInterval = setInterval(() => {
totalSeconds--;

if (totalSeconds < 0) {
clearInterval(countdownInterval);
timeCountdown.innerText = "Time's up!";
document.body.style.backgroundColor = "#ff4c4c";
playAlarm();
return;
}

updateDisplay();
}, 1000);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you improve the indentation of this code?


// Validate the input
if (isNaN(totalSeconds) || totalSeconds < 0) {
timeCountdown.innerText = "Please enter a valid number of seconds.";
Copy link
Copy Markdown
Contributor

@cjyuan cjyuan Jul 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you ever see your app displaying ""Please enter a valid number of seconds."? (If not, it may imply the code is not needed)

What does the function setAlarm() do? (It currently does not actually set any alarm)
The code that actually set the alarm is in startCountDown().

If you combine the code in setAlarm() and the code in startCountDown(), you should discover some redundant code.

Copy link
Copy Markdown
Author

@delmorallopez delmorallopez Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified setalarm(), where the function will call updateDisplay()....
No redundancy code now!

I commited the last changes....

I am struggling to changes made in the previous commit in order to remove the quote generator files... I will ask for help on Saturday to a volunteer

@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 27, 2025
@delmorallopez delmorallopez added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 28, 2025
Comment thread Sprint-3/alarmclock/alarmclock.js Outdated
Comment thread Sprint-3/alarmclock/alarmclock.js
Comment thread Sprint-3/alarmclock/alarmclock.js Outdated
Comment thread Sprint-3/alarmclock/alarmclock.js Outdated
Comment on lines +31 to +40
function startCountdown() {
let totalSeconds = isValidInput(timeSetup.value);

if (totalSeconds === null) {
timeCountdown.innerText = "Time Remaining: 00:00";
return;
}

clearInterval(countdownInterval); // Stop previous countdown
updateDisplay(totalSeconds); // Show first tick immediately
Copy link
Copy Markdown
Contributor

@cjyuan cjyuan Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can consider implementing startCountdown() in the following way:

function startCountDown(totalSeconds) {  // take a parameter
  // Delete the code on lines 32-38, 40 (The code in setAlarm() has performed those tasks)
 ..
}

Then instead of calling startCountDown() on line 65, call it at the end of setAlarm() with the appropriate argument.

@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 28, 2025
@delmorallopez delmorallopez added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 28, 2025
Copy link
Copy Markdown
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.

I just noticed currently when the timer reaches zero, the alarm will only sound 1 second later (and not immediately). Can you address this issue?

Comment thread Sprint-3/alarmclock/alarmclock.js Outdated
}

startCountdown(totalSeconds); // Start the countdown timer
updateDisplay(totalSeconds);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are already calling updateDisplay() in startCoutndown(). There is no need to call it again.

@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 30, 2025
@delmorallopez
Copy link
Copy Markdown
Author

I moved the totalSeconds-- after the if check. That way, when totalSeconds reaches 0, the alarm triggers immediately.

@delmorallopez delmorallopez added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 30, 2025
@cjyuan
Copy link
Copy Markdown
Contributor

cjyuan commented Jul 30, 2025

Did you push you changes to GitHub?

@delmorallopez
Copy link
Copy Markdown
Author

The changes has been pushed

@cjyuan
Copy link
Copy Markdown
Contributor

cjyuan commented Jul 30, 2025

The latest copy of the file you pushed to GitHub looks like an older version of the file. The recent changes you made are all gone. The current version is like the first version I reviewed. Can you double check to see if you have submitted the correct file?

@delmorallopez
Copy link
Copy Markdown
Author

Last commit has been uploaded, I am sorry @cjyuan

@cjyuan
Copy link
Copy Markdown
Contributor

cjyuan commented Aug 1, 2025

Changes look good. Well done

@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 Aug 1, 2025
if (!/^\d+$/.test(trimmed)) return null;

const num = Number(trimmed);
return num > 0 ? num : null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think NaN is better than null because NaN is also of type number.

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants