Skip to content

London| 26-ITP-May | Mandip Sanger | Sprint 3 | Alarm Clock - #1435

Open
mandipsanger wants to merge 8 commits into
CodeYourFuture:mainfrom
mandipsanger:Sprint-3
Open

London| 26-ITP-May | Mandip Sanger | Sprint 3 | Alarm Clock#1435
mandipsanger wants to merge 8 commits into
CodeYourFuture:mainfrom
mandipsanger:Sprint-3

Conversation

@mandipsanger

@mandipsanger mandipsanger commented Aug 14, 2026 •
edited
Loading

Copy link
Copy Markdown

Self checklist

Sprint 3 AlarmClock

@github-actions

This comment has been minimized.

Sign in to view
@mandipsanger mandipsanger added 📅 Sprint 3 Assigned during Sprint 3 of this module Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 14, 2026
cjyuan
cjyuan reviewed Aug 14, 2026
View reviewed changes

@cjyuan cjyuan left a comment

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 missed updating index.html according to an instruction in readme.md.

  • Currently when starting a new countdown, the application does not always return to a clean initial state, which can lead to inconsistent behaviour between runs.

    Note: a user may not click the "Stop Alarm" button first before starting a new count down.

Comment thread Sprint-3/alarmclock/alarmclock.js Outdated
@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 Aug 14, 2026
@mandipsanger mandipsanger added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Aug 18, 2026
cjyuan
cjyuan reviewed Aug 18, 2026
View reviewed changes
Comment thread Sprint-3/alarmclock/index.html
<label for="alarmSet">Set time to:</label>
<input id="alarmSet" type="number" />
<label for="alarmSet">Set time in seconds:</label>
<input id="alarmSet" type="number" min="1" step="1" placeholder="Enter seconds" />

@cjyuan cjyuan Aug 18, 2026

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.

Note: Without a form, the browser won't check the input against the constraints min="1" step="1".

Change is optional.

Comment thread Sprint-3/alarmclock/alarmclock.js Outdated
Comment thread Sprint-3/alarmclock/alarmclock.js
@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 Aug 18, 2026
cjyuan
cjyuan reviewed Aug 20, 2026
View reviewed changes

@cjyuan cjyuan left a comment

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 assume this PR is ready to be re-reviewed.

Changes look good.

Comment thread Sprint-3/alarmclock/alarmclock.js
Comment on lines 17 to 26
if (!Number.isInteger(numberValue)) {
alert("only enter an integer value.");
return; // exit a function if not an integer
alert("Only enter an integer value.");
return;
}

//step 4 validate if it is greater than 0
// Step 4: validate if it is greater than 0
if (numberValue <= 0) {
alert("only enter a number which is bigger than 0");
return; //Exit a funciton if not greater than 0
alert("Only enter a number which is bigger than 0.");
return;
}

@cjyuan cjyuan Aug 20, 2026

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.

Notes:

  • Could probably just check both conditions in the one if-statement and then change the message to mention "... positive integer".

  • While alert() is convenient to use, it is not a user-friendly way to notify the user an error.

No change needed.

@cjyuan cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Aug 20, 2026
@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown

The files changed in this PR don't match what is expected for this task.

Please check that you committed the right files for the task, and that there are no accidentally committed files from other sprints.

Please review the 'files changed' tab at the top of the page.

Here is an example of a file that has been changed on this branch but shouldn't be: Sprint-3/todo-list/script.mjs

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above.

1 similar comment
@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown

The files changed in this PR don't match what is expected for this task.

Please check that you committed the right files for the task, and that there are no accidentally committed files from other sprints.

Please review the 'files changed' tab at the top of the page.

Here is an example of a file that has been changed on this branch but shouldn't be: Sprint-3/todo-list/script.mjs

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above.

@cjyuan

cjyuan commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

In your most recent commit, you introduced a modified file in the todo-list folder. Could your revert the change to that file to keep this PR clean?

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Complete Volunteer to add when work is complete and all review comments have been addressed. labels Aug 20, 2026
@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown

The files changed in this PR don't match what is expected for this task.

Please check that you committed the right files for the task, and that there are no accidentally committed files from other sprints.

Please review the 'files changed' tab at the top of the page.

Here is an example of a file that has been changed on this branch but shouldn't be: Sprint-3/todo-list/script.mjs

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above.

1 similar comment
@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown

The files changed in this PR don't match what is expected for this task.

Please check that you committed the right files for the task, and that there are no accidentally committed files from other sprints.

Please review the 'files changed' tab at the top of the page.

Here is an example of a file that has been changed on this branch but shouldn't be: Sprint-3/todo-list/script.mjs

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjyuan cjyuan cjyuan left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

Reviewed Volunteer to add when completing a review with trainee action still to take. 📅 Sprint 3 Assigned during Sprint 3 of this module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mandipsanger @cjyuan