Skip to content

WESTMIDLANDS | ITP-MAY2025 | Peter Lui | Sprint3 Alarm Clock #738

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

WESTMIDLANDS | ITP-MAY2025 | Peter Lui | Sprint3 Alarm Clock #738

wants to merge 3 commits into from
+52 −2

Conversation

petergmlui
Copy link

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Initial submission of Alarm Clock and updated index.html

Questions

Ask any questions you have for your reviewer.

@petergmlui petergmlui added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 2, 2025
@arun-john arun-john added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 4, 2025
@arun-john
Copy link

Good work on this Peter!

The jest tests all pass and manual checks work well...I noticed a minor bug though. The alarm seems to take a extra second to go off after it reaches 00:00? Pls. check

I like how you have broken out the logic into functions and also your comments, making it much easier to read!

Reg. the pad function - You could also do this with String padStart(), but this is fine too.

@petergmlui
Update alarmclock.js
8aec6dd 
fixed alarm delay
@petergmlui petergmlui added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Aug 6, 2025
@petergmlui
Copy link
Author

Good work on this Peter!

The jest tests all pass and manual checks work well...I noticed a minor bug though. The alarm seems to take a extra second to go off after it reaches 00:00? Pls. check

I like how you have broken out the logic into functions and also your comments, making it much easier to read!

Reg. the pad function - You could also do this with String padStart(), but this is fine too.

Thanks John! I have updated the code to check the timer first - that should fix the delay. Have a great day!

@arun-john
Copy link

Did you try rerunning the tests (npm test) and doing manual checks following your change? I am getting some failures for the automated tests...

@petergmlui
Copy link
Author

@arun-john Thank you for noticing. I have fixed and tested again and it works now.

@arun-john
Copy link

Great, all working now! 👍

@arun-john arun-john 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. labels Aug 9, 2025
@petergmlui petergmlui mentioned this pull request Aug 9, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Complete Volunteer to add when work is complete and all review comments have been addressed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@petergmlui @arun-john