Skip to content

Glasgow | ITP May 25 | Mirabelle Morah | Data Groups | Sprint-3 | Alarm Clock #603

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mirabellemorah
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_NAME | FIRST_NAME LAST_NAME | PROJ_NAME
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

  1. I adjusted the CSS to create a new one
  2. Updated javascript

Questions

Nil. Thank you.

@mirabellemorah mirabellemorah added the Needs Review Participant to add when requesting review label Jul 14, 2025
@YoanHlebarov YoanHlebarov added the Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. label Jul 19, 2025
Copy link

@YoanHlebarov YoanHlebarov left a comment

Choose a reason for hiding this comment

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

Well done! You have done a good job at finishing the code for the alarm. A couple of small comments that you should resolve easily.

I would consider two ideas that I think would be useful for you. You clearly have a good understanding of the code at this stage, so you can think about user experience and code readability.

User Experience is very important, you want to make sure your customers want and enjoy to use the products you create. Think how you use apps, what kind of feedback is there when you take an action? How do you see errors? What happens when you try to do something that you can't/not allowed to? Think how these are handled by other well designed apps and how you can integrate that in your work.

Code readability is another one of those "nice to do" things. If others are reading your code, you want to make sure it is easily understood and it makes sense. Often times you may find yourself able to achieve something in code with a single line. That sounds great, but you should always consider if it is easy to read for others, or even for yourself a year down the line.

Comment on lines +20 to +24
if (isNaN(currentTime) || currentTime <= 0) {
document.getElementById("timeRemaining").innerText =
"Time Remaining: 00:00";
return;
}

Choose a reason for hiding this comment

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

I would also consider resetting the input to be empty. This way you can let the user know that an action has indeed happened, but it did not resolve into any meaningful result as the input was not valid.

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 put this into consideration and actioned.

Comment on lines 4 to 7
margin: 0;
padding: 0;
font-family: rubik, sans-serif;
margin: 10px 0;

Choose a reason for hiding this comment

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

You don't need two margin declarations here. See which one you need. As a side question - which of these two do you think will take precedence here?

Copy link
Author

Choose a reason for hiding this comment

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

You're right that was an oversight. The last margin will take effect "margin: 10px 0; because it's being read in a 'cascading' format. I have taken off the first one

@YoanHlebarov YoanHlebarov added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Jul 19, 2025
@mirabellemorah mirabellemorah added the Needs Review Participant to add when requesting review label Jul 21, 2025
@mirabellemorah
Copy link
Author

Thank you for the feedback! I have taken off the extra margin and made the input box reset to "empty" once "set alarm" button is hit, completed the two comments now. On your feedback on UX and code understanding, I will keep improving on this. Thanks.

Well done! You have done a good job at finishing the code for the alarm. A couple of small comments that you should resolve easily.

I would consider two ideas that I think would be useful for you. You clearly have a good understanding of the code at this stage, so you can think about user experience and code readability.

User Experience is very important, you want to make sure your customers want and enjoy to use the products you create. Think how you use apps, what kind of feedback is there when you take an action? How do you see errors? What happens when you try to do something that you can't/not allowed to? Think how these are handled by other well designed apps and how you can integrate that in your work.

Code readability is another one of those "nice to do" things. If others are reading your code, you want to make sure it is easily understood and it makes sense. Often times you may find yourself able to achieve something in code with a single line. That sounds great, but you should always consider if it is easy to read for others, or even for yourself a year down the line.

@YoanHlebarov
Copy link

Thank you for addressing those. Keep up the great work! I think this covers all requirements for the task so I shall mark it as complete.

@YoanHlebarov YoanHlebarov added Complete Volunteer to add when work is complete and review comments have been addressed and removed Needs Review Participant to add when requesting review Reviewed Volunteer to add when completing a review labels Jul 22, 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 review comments have been addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants