Skip to content

Conversation

@Baba05206
Copy link

@Baba05206 Baba05206 commented Nov 30, 2025

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

I implemented the setAlarm() function as per instruction

Set Alarm sets Time Remaining to the MM:SS equivalent of the user input in field 'Set time to'
The function decrements Time Remaining: every second i.e. counted down by 1 sec
When Time Remaining: reaches 00:00, the alarm plays a sound.
Stop Alarm stops the alarm sound being played
Checked for update to the testing framework by running npm install
Updated the title in the index.html file as per task instruction
and then...
I tried to change branch name but got it all wrong
and then i tried again a

Questions

@github-actions
Copy link

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

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.

@Baba05206 Baba05206 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. Module-Data-Groups The name of the module. labels Nov 30, 2025
@github-actions
Copy link

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

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
Copy link

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

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.

@github-actions github-actions bot removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Nov 30, 2025
@github-actions
Copy link

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

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.

@Baba05206 Baba05206 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Nov 30, 2025
@github-actions
Copy link

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

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.

@github-actions github-actions bot removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Nov 30, 2025
@Baba05206
Copy link
Author

@cjyuan , i need some help here. You already reviewed and commented on my first submission. I made several errors in attempting to change the branch name hence the multiple submissions and all the errors. May I ask for your help to walk me through how to deal with this sort of scenario in the future?

@Baba05206 Baba05206 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Nov 30, 2025
@github-actions
Copy link

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

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.

@github-actions github-actions bot removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Nov 30, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Nov 30, 2025

Feel free to DM me on Slack if you would like to arrangement an online meeting with me.

If you don't have any questions to ask, you should delete the "Questions" section or change the default text in the "Questions" section in the PR description. Otherwise the bot will remove the "Needs review" label.

image

@Baba05206 Baba05206 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Nov 30, 2025
<div class="centre">
<h1 id="timeRemaining">Time Remaining: 00:00</h1>
<label for="alarmSet">Set time to:</label>
<input id="alarmSet" type="number" />
Copy link

Choose a reason for hiding this comment

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

When the user first sees this field, how would he know what to input is it minute, second, hours?, and also think about the screen readers, give as much as context to the user programmatically as well as visually.

Copy link
Author

Choose a reason for hiding this comment

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

I have used a representative name totalSeconds to show input is in seconds.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Baba05206 I think the original comment is about improving accessibility in the HTML code.

Can you try sharing your HTML code to an AI, ask it how you can improve its accessibility, and then update your code accordingly?


function setAlarm() {
const input = document.getElementById("alarmSet");
let time = Number(input.value);
Copy link

Choose a reason for hiding this comment

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

Is the variable readable, time (is it hour, minute, second, milliseconds)?

Copy link
Author

Choose a reason for hiding this comment

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

now, i believe it is. i had thought it did, but now certainly does. thanks


function setAlarm() {
const input = document.getElementById("alarmSet");
let time = Number(input.value);
Copy link

Choose a reason for hiding this comment

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

What if the user inputs -1?

Copy link
Author

Choose a reason for hiding this comment

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

will review, update based on the above comments and revert by tomorrow morning. many thanks for for your guidance.

Comment on lines 11 to 14
function format(num) {
if (num < 10) return "0" + num;
return num;
}
Copy link

Choose a reason for hiding this comment

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

This is nice way to pad number, is there any predefined methods?

Copy link
Author

@Baba05206 Baba05206 Dec 7, 2025

Choose a reason for hiding this comment

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

... i have now changed to code to use padStart instead of format()

Comment on lines 34 to 37
format(Math.floor(time / 60)) +
":" +
format(time % 60);
}, 1000);
Copy link

Choose a reason for hiding this comment

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

These 60 and 1000 are magic numbers, is there better way to handle magic numbers?

Copy link
Author

Choose a reason for hiding this comment

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

Replaced magic numbers with named constants that explains their purpose.

Comment on lines 18 to 20
format(Math.floor(time / 60)) +
":" +
format(time % 60);
Copy link

Choose a reason for hiding this comment

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

You are repeating this twice in your code, how would you increase efficiency if you needed to change something in the future?

Copy link
Author

@Baba05206 Baba05206 Dec 7, 2025

Choose a reason for hiding this comment

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

I can hold the value in/as a variable so I can call the variable without having to repeat an expression or an existing value.

Copy link
Author

Choose a reason for hiding this comment

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

To cater for invalid entry (include negative values), I am now implemented a form of input validation to ensure the user enters a valid positive number.

Copy link

Choose a reason for hiding this comment

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

Hi @Baba05206 , wouldn't it be easy to create a utility function, that takes arguments like time and then the return value is the string with formatted time and second, and then you can just call this function where you need!

@bp7968h bp7968h 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 Dec 3, 2025
@Baba05206 Baba05206 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Dec 7, 2025
@bp7968h bp7968h removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Dec 11, 2025
@Baba05206 Baba05206 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Dec 11, 2025
@Baba05206
Copy link
Author

@cjyuan , may I ask for your feedback on the updated response please? Thank you.

Copy link
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 think you have addressed all comments (except the improving the accessibility in HTML part) by @bp7968h.

In addition to improving accessibility, can you also address my two new comments?

Comment on lines 26 to 34
const heading = document.getElementById("timeRemaining");

let totalSeconds = Number(input.value);

// Input validation: check for invalid or non-positive numbers
if (isNaN(totalSeconds) || totalSeconds <= 0) {
heading.innerText = "Please enter a valid positive number of seconds";
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Some input that can mess up the clock display can still pass this check.

@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Dec 13, 2025
…nt invalid inputs and reset clock when new input is accepted
@Baba05206 Baba05206 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Dec 13, 2025
Copy link
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.

Changes look good.

Just a few more suggestions. No change required.

Comment on lines +53 to +54
// Reset any existing countdown
clearInterval(intervalId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could consider placing all the "resetting" code together (code on lines 54 and 28).

Comment on lines +12 to +14
<h1 id="timeRemaining" aria-live="polite" role="timer">
Time Remaining: 00:00
</h1>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could consider express code on lines 12-14 as:

      <div id="timeRemaining" aria-live="polite" role="timer">
        Time Remaining: <span id="theTime">00:00</span>
      </div>

Note: Use AI to find out why if needed.


// Prevent extremely large or zero values
if (totalSeconds === 0 || totalSeconds > 86400) {
heading.innerText = "Time Remaining: 00:00";
Copy link
Contributor

Choose a reason for hiding this comment

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

The same time-update code shows up in multiple places, which is a good signal that it could be factored into a utility function.

Comment on lines +35 to +46
// STRONG VALIDATION: must be digits only
if (!/^\d+$/.test(raw)) {
heading.innerText = "Time Remaining: 00:00";
errorMsg.textContent =
"Invalid input. Please enter a whole number of seconds (e.g., 10, 30, 120). Decimals and text are not allowed.";
return;
}

let totalSeconds = Number(raw);

// Prevent extremely large or zero values
if (totalSeconds === 0 || totalSeconds > 86400) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could consider combining these two checks into:

 let totalSeconds = Number(raw);
 if (!Number.isInteger(totalSeconds) || totalSeconds <= 0 || totalSeconds > 86400) {
    errorMsg.textContent = "Please enter an integer between 1 and 86400";
    return;
 }

@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 Dec 13, 2025
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. Module-Data-Groups The name of the module. 📅 Sprint 3 Assigned during Sprint 3 of this module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants