-
-
Notifications
You must be signed in to change notification settings - Fork 207
West Midlands | 25-ITP-Sep | Baba Yusuf | Sprint 3 | Alarm Clock #900
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
Conversation
|
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. |
|
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
|
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. |
|
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. |
|
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. |
|
@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? |
|
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. |
Sprint-3/alarmclock/index.html
Outdated
| <div class="centre"> | ||
| <h1 id="timeRemaining">Time Remaining: 00:00</h1> | ||
| <label for="alarmSet">Set time to:</label> | ||
| <input id="alarmSet" type="number" /> |
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.
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.
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.
I have used a representative name totalSeconds to show input is in seconds.
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.
@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?
Sprint-3/alarmclock/alarmclock.js
Outdated
|
|
||
| function setAlarm() { | ||
| const input = document.getElementById("alarmSet"); | ||
| let time = Number(input.value); |
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.
Is the variable readable, time (is it hour, minute, second, milliseconds)?
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.
now, i believe it is. i had thought it did, but now certainly does. thanks
Sprint-3/alarmclock/alarmclock.js
Outdated
|
|
||
| function setAlarm() { | ||
| const input = document.getElementById("alarmSet"); | ||
| let time = Number(input.value); |
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.
What if the user inputs -1?
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.
will review, update based on the above comments and revert by tomorrow morning. many thanks for for your guidance.
Sprint-3/alarmclock/alarmclock.js
Outdated
| function format(num) { | ||
| if (num < 10) return "0" + num; | ||
| return num; | ||
| } |
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.
This is nice way to pad number, is there any predefined methods?
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.
... i have now changed to code to use padStart instead of format()
Sprint-3/alarmclock/alarmclock.js
Outdated
| format(Math.floor(time / 60)) + | ||
| ":" + | ||
| format(time % 60); | ||
| }, 1000); |
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.
These 60 and 1000 are magic numbers, is there better way to handle magic numbers?
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.
Replaced magic numbers with named constants that explains their purpose.
Sprint-3/alarmclock/alarmclock.js
Outdated
| format(Math.floor(time / 60)) + | ||
| ":" + | ||
| format(time % 60); |
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.
You are repeating this twice in your code, how would you increase efficiency if you needed to change something in the future?
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.
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.
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.
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.
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.
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!
|
@cjyuan , may I ask for your feedback on the updated response please? Thank you. |
cjyuan
left a comment
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.
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?
| 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; | ||
| } |
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.
Some input that can mess up the clock display can still pass this check.
…nt invalid inputs and reset clock when new input is accepted
cjyuan
left a comment
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.
Changes look good.
Just a few more suggestions. No change required.
| // Reset any existing countdown | ||
| 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.
Could consider placing all the "resetting" code together (code on lines 54 and 28).
| <h1 id="timeRemaining" aria-live="polite" role="timer"> | ||
| Time Remaining: 00:00 | ||
| </h1> |
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.
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"; |
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.
The same time-update code shows up in multiple places, which is a good signal that it could be factored into a utility function.
| // 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) { |
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.
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;
}

Learners, PR Template
Self checklist
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