-
-
Notifications
You must be signed in to change notification settings - Fork 191
West Midlands | 25 Sep ITP | Iswat Bello | Sprint 3 | Build alarm clock app #865
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
…red time-formatting logic into an updateDisplay helper function
…meRemaining title background to blue at end and reset to default at start
| let intervalId; | ||
|
|
||
| // Helper function to format total seconds and update the UI | ||
| function updateDisplay(totalSeconds) { |
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.
Nice use of a helper function @Iswanna
Separating the display formatting into its own function is excellent practice and keeps setAlarm() cleaner.
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 for the feedback @jaymes15.
| let totalSeconds = Number(inputElement.value); //this converts the input from string to number | ||
|
|
||
| // Validate input | ||
| if (totalSeconds <= 0 || isNaN(totalSeconds)) { |
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 validation!
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!
| // the code you write here will run every one second or 1000 milliseconds | ||
| totalSeconds = totalSeconds - 1; | ||
| if (totalSeconds <= 0) { | ||
| clearInterval(intervalId); |
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.
Correct use of clearInterval
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
Sprint-3/alarmclock/alarmclock.js
Outdated
| clearInterval(intervalId); | ||
| playAlarm(); | ||
| document.getElementById("timeRemaining").textContent = | ||
| "Time Remaining: 00:00"; |
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.
@Iswanna Can you think of a way to avoid having Time Remaining: in multiple places in your code
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.
@jaymes15, if I store Time Remaining: in a global variable, would that be okay?
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.
That works @Iswanna
|
Hi @jaymes15 . I have updated my code with the changes. Please kindly review my PR. Thank you. |
Learners, PR Template
Self checklist
Changelist
In this pull request, I completed the alarm clock project by implementing the setAlarm functionality with input validation, a live countdown in mm:ss format, and alarm sound at 00:00. I added a helper function to update the display, clear the input, and change the background color when the countdown finishes.
Questions
Hi. Please could you review my PR? I’d really appreciate your feedback.