-
-
Notifications
You must be signed in to change notification settings - Fork 405
london | may2025 | mohammad | aldali | form-controls #475
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
london | may2025 | mohammad | aldali | form-controls #475
Conversation
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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.
A well-prepared PR makes it easier for reviewers to approve it with minimal back-and-forth.
The "PR Essentials" file on the Canvas in the #cyf-code-review channel has some helpful tips to make your PRs more robust and ready for review.
Can you take a few minutes to check against the essentials, including how to prepare PR description? Doing so can help speed up the review process.
And as a practice to get better at using AI tools, try running your code by ChatGPT. You can ask ChatGPT
review HTML code:
... <--- paste your HTML code here
Then take a look at the suggestions it gives you, and pick out the ones that actually make sense for your project. It's a great way to learn and improve your work.
Once you've finished making changes, please leave a comment on this PR so I’ll get notified. Thanks!
|
hi #cjyuan I've reviewed the code with ChatGPT, cleaned up the structure, and added proper validations. Looking forward to your feedback! |
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.
You are off to a solid start.
Can you address the following issues and the comments I left with the code to further improve your implementation?
-
While there is no error, W3C Markup Validation Service shows some warnings in your code. Can you address those warnings?
-
The Lighthouse accessibility score is high but not yet 100. Can you improve the score to 100?
-
In your PR description, in the "Changelist" section, can you briefly describe what you have implemented in this PR branch?
Form-Controls/index.html
Outdated
| <label for="password">Password:</label> | ||
| <span aria-label="required"><strong>*</strong></span> | ||
| <input type="password" id="password" name="password" placeholder="Enter your password" minlength="8" 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.
It’s important to ensure the implementation adheres to the specifications outlined in the README.md to maintain consistency and avoid unexpected behavior. If you’d like to introduce new features you believe are useful, it’s best to request pre-approval first to ensure alignment with project goals.
Form-Controls/index.html
Outdated
| <section> | ||
| <h2>Choose your T-shirt</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.
For forms, using <fieldset> with <legend> are specifically designed for grouping form controls and labeling them. As such, they are more semantic than using <section> with <h2>.
Form-Controls/index.html
Outdated
|
|
||
| <section> | ||
| <h2>Choose your T-shirt</h2> | ||
| <p>Please fill all required information</p> |
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.
Why place this instruction in the middle of the form when all fields are required?
Form-Controls/index.html
Outdated
| <section> | ||
| <label for="name">Your name:</label> | ||
| <span aria-label="required"><strong> * </strong></span> | ||
| <input type="text" id="name" name="user-name" placeholder="Enter your name" 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.
Currently a user can enter a name consisting of only space characters (e.g., " "). Can you enforce a stricter validation rule using the pattern attribute to disallow any name that contains only space characters?
…idation, and cleaned up HTML
|
Thanks for the feedback! I removed the password field as it wasn’t required. I cleaned up the HTML using only semantic elements like fieldset and legend. Let me know if anything else needs fixing. |
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.
Code are greatly improved! Well done!
Can you address the following issues to further improve your implementations?
-
The Lighthouse accessibility score is 95. The spec asks for 100. Can you improve this score to 100?
-
Can you also address the warning indicated by the W3C Markup Validation Service?
-
To follow best practices, make sure to
- Check the items in the Self-Checklist to confirm your pull request meets the guidelines (which you have done! Good!)
- Provide a brief description (under the "Changelist" section) summarizing the purpose of the PR and the changes you’ve made
… requirements: - Removed unnecessary ARIA attributes to avoid validation issues. - Added proper `<label>` elements for all inputs and selects. - Adjusted the input, select, and button sizes to meet the required touch target size (min 44px height). - Updated the "Choose your size" label and fixed the placeholder text. - Re-tested with Lighthouse and now the accessibility score is 100%. Let me know if anything else needs updating. Thanks!
|
The errors I see are these: This is the file I checked: https://github.com/CodeYourFuture/Module-Onboarding/blob/7c5825928d00e2e122f5af924bf14e325d838b3b/Form-Controls/index.html |
|
I’ve removed the stray |
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.
According to https://validator.w3.org/, there is still an error in your code. Can you fix it?
You can use the above website to check for errors yourself.
Form-Controls/index.html
Outdated
|
|
||
| </select> | ||
| <hr> | ||
| <label for="size" >choose your size: </label> |
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.
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've done ,
could you chek please?
|
I'VE DONE |
|
Changes look good. |





Learners, PR Template
Self checklist
Changelist
Questions
Ask any questions you have for your reviewer.