-
-
Notifications
You must be signed in to change notification settings - Fork 420
Glasgow | ITP January-2026| Tuan Nguyen | Sprint 2| Form control #1103
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
base: main
Are you sure you want to change the base?
Glasgow | ITP January-2026| Tuan Nguyen | Sprint 2| Form control #1103
Conversation
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
jenny-alexander
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.
Nice work @Jacknguyen4438. I left a comments for you to review.
Form-Controls/index.html
Outdated
| <footer> | ||
| <!-- change to your name--> | ||
| <h2>By HOMEWORK SOLUTION</h2> | ||
| <h2>By Tuan Nguyen</h2> |
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 suggest using something other than <h2> in the footer. By using <h2> you are announcing a major section header within your footer.
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.
Thank you for the comment, I will find some other tag that are correct.
Form-Controls/index.html
Outdated
| <br> | ||
| <div> | ||
| <label for="Color">T-shirt color:</label> | ||
| <input type="color" id="Color" name=" Product color" required> |
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.
Wow 🤩, I really like this unique choice you made with the color picker. Very interesting.
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.
Thank you, I choose this because I I think is easier and provide the user choice more precise.
Form-Controls/index.html
Outdated
| @@ -17,11 +17,41 @@ <h1>Product Pick</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.
I think these comments can be removed since their intention was to help you complete the exercise. Comments in the code can be useful for:
- Describing complex logic or workarounds
- Explaining "why," not "what"
- TODO notes for incomplete work
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.
Can you review the exercise requirements? Should there be changes in the README file?
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 check the criteria and make some change. I am happy report that the change now fit with the course check list.
Form-Controls/index.html
Outdated
| <br> | ||
| <div> | ||
| <label for="Color">T-shirt color:</label> | ||
| <input type="color" id="Color" name=" Product color" required> |
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 see a leading space in the name=" Product color". This could cause issues if the form was ever submitted for processing.
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.
Thank you for point this out, this is some gramma mistake I have fix this and it shouldn't be a problem.
Form-Controls/index.html
Outdated
| </div> | ||
| <div> | ||
| <br> | ||
| <label for="Size">Shirt size |
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 see something missing on line 36. Can you find the problem?
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.
Yes I have check and see that is should be after size word and not nested the whole select element.
…rror and update the code for better reading
jenny-alexander
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.
@Jacknguyen4438 Nice work making those code corrections.
Can you review the<label> attributes? Specifically check the casing of your for attributes. It's best practice to user lowercase (even though uppercase is technically ok).
https://courses.cs.washington.edu/courses/cse154/codequalityguide/html/#lowercase-elements

Learners, PR Template
Self checklist
Changelist
I have added input allow for the user submit their data through the form website.
Questions
No question