London | 26-ITP-JAN | Asha Ahmed | Sprint 3 | Alarm clock

[ashaahmed7]

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

• Updated index.html title to “Alarm clock app”
• Implemented setAlarm() logic in alarmclock.js
• Added time formatting to display input in mm:ss
• Implemented countdown functionality using setInterval
• Triggered playAlarm() when timer reaches 00:00
• Cleared interval when countdown completes to prevent negative values
• Updated the title
• Amended comments as requested

Questions

N/A

[hkavalikas]

This doesn't quite match the title requirement we got from the readme, let's match it exactly.

[ashaahmed7]

I will make the change to "Alarm clock app" and make a commit.

[hkavalikas]

This comment isn't quite accurate, can you think of a way to improve it?

[ashaahmed7]

The comment can be improved as follows:
// Store the remaining time; currently this value is in minutes (not seconds yet)

[hkavalikas]

Can you walk me through what will happen on every "tick" from 2 seconds down to 0? Is there something in this statement we can remove that might be redundant?

[ashaahmed7]

From 2 → 1 nothing happens in this block, and when it reaches 0 the condition becomes true and the interval is cleared, display updated, and alarm played. The assignment secondsLeft = 0 is redundant here because the value is already 0 when the condition is met (assuming it doesn’t go negative), so it can be removed.

[hkavalikas]

The comment and variable aren't quite right. Can you spot what is wrong?

[ashaahmed7]

The code actually reads whatever is in the element with id alarmSet
// Read the number of minutes from the input field (expects minutes)

[hkavalikas]

If we were to write 5 on that field, would the alarm go and count up to five minutes?

[ashaahmed7]

No, it will count down from 5 seconds instead.

[hkavalikas]

Finally, let's try to add a few more details on the Changelist. The purpose of the changelist is to mention individual updates/additions/changes made for this PR, not so much to add a headline (like the title) of the changes.

[hkavalikas]

Quick nudge @ashaahmed7 in case you missed my comments last week.

[ashaahmed7]

@hkavalikas Sorry I hadn't had the chance to address your comments, its been a busy week. I've made changes to comments, edited Changelist and the title as requested.

[hkavalikas]

The id of this item won't change per "tick". How can we reuse it instead of trying to get the DOM item every time?

[hkavalikas]

For visibility, also replied here:
#1009 (comment)

[hkavalikas]

Setting a new alarm doesn't silence an alarm that is already ringing. How can we make sure the alarm sound stops when we override it with a new one?

[hkavalikas]

It's an optional task. The "Stop alarm" pauses the sound, which is great. How can we make it pause the alarm as well? e.g. if you press it at 5 seconds, it should stop counting at 5 seconds.

Additionally, if you want to try it, how can we make the background flash, or at least change colours when the alarm rings (and we can reset that when the user clicks on "Stop alarm".

For what is worth, both of the above tasks are under the Extra Tasks of the readme, and they aren't a hard requirement.

[ashaahmed7]

Setting a new alarm doesn't silence an alarm that is already ringing. How can we make sure the alarm sound stops when we override it with a new one?

Setting a new alarm doesn't silence an alarm that is already ringing. How can we make sure the alarm sound stops when we override it with a new one?

To make sure the previous alarm sound stops when you “override” it by setting a new alarm, you need to stop/reset the audio at the moment you set a new timer (and ideally also reset it when you press Stop).

Right now, setAlarm() clears the countdown interval, but it doesn’t stop the audio if it’s already playing. So the old alarm keeps sounding even though you’ve started a new countdown.

I can dd a small helper that fully resets the audio (pause + rewind). Then call it at the top of setAlarm().

Changes to make

var audio = new Audio("alarmsound.mp3");

function stopAndResetAlarm() {
  audio.pause();
  audio.currentTime = 0;   // rewind to start so next play starts cleanly
}

Now call it inside setAlarm() before starting the new countdown:

function setAlarm() {
  // Always stop any currently playing alarm when overriding
  stopAndResetAlarm();

  const input = document.getElementById("alarmSet");
  const seconds = parseInt(input.value, 10);

  if (isNaN(seconds) || seconds <= 0) {
    return;
  }

  secondsLeft = seconds;

  if (countdownInterval !== null) {
    clearInterval(countdownInterval);
    countdownInterval = null;
  }

  updateTimeDisplay();

  countdownInterval = setInterval(() => {
    secondsLeft = secondsLeft - 1;
    updateTimeDisplay();

    if (secondsLeft <= 0) {
      clearInterval(countdownInterval);
      countdownInterval = null;
      secondsLeft = 0;
      updateTimeDisplay();
      playAlarm();
    }
  }, 1000);
}

[ashaahmed7]

@cjyuan Could you kindly take a look at this PR for me? It’s the only thing holding me back from submitting Module 3.

[cjyuan]

@ashaahmed7

• The ITP deadline is 23rd April. As long as you can complete this PR by 23rd Apr, it will be ok.

• Your proposed change seems to have addressed the comment about stopping the audio when starting a new countdown. Why not commit the change?

• Since your response was made 18 hours ago, you should give @hkavalikas some more time to react.

Note: You can tag me again in 2 days if this PR is still not yet re-reviewed by then.

[hkavalikas]

@ashaahmed7 Re the urgency of the review, I am checking PRs once daily, as you can imagine, comments sometimes take days or even weeks to be addressed/replied to and as volunteers, we are trying our best to stay on top of it.

Having said that, I'm more than happy to re-review, but as @cjyuan mentioned, nothing was really committed yet, we don't typically review the comments themselves (unless of course there is a question and/or concern in the thread), once you test your solution, you are happy with it and commit it, feel free to update the label again and I will have a look.

@cjyuan thank you for jumping on it so quickly 🙏

[ashaahmed7]

Hi @hkavalikas & @cjyuan — apologies, I had mistakenly thought the deadline was the 19th, which is why I panicked earlier.

I’ve now made the necessary changes and hope it meets the requirements.

[hkavalikas]

Hi @hkavalikas & @cjyuan — apologies, I had mistakenly thought the deadline was the 19th, which is why I panicked earlier.

I’ve now made the necessary changes and hope it meets the requirements.

Don't mention it, thank you for chasing it up.

[illicitonion]

Closing PR because the January ITP run has finished. Feel free to re-open if you're still working on it.