West Midlands | 26-Jan-ITP | Fida H Ali Zada | Sprint 2 | Form Controls #925
Conversation
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
alizada-dev
added
Needs Review
jenny-alexander
added
Review in progress
|
Hi @jenny-alexander, can you please review this PR? As it has been under review for progress since yesterday. Thank you! |
|
If all the requirements are met, can you please change the review to 'complete'? |
I appreciate you wanting to move quickly on this! Just so you know, I typically try to review PRs within 24-48 hours after assigning them to myself. |
jenny-alexander
left a comment
jenny-alexander
left a comment
There was a problem hiding this comment.
@alizada-dev - nice work with the form control exercise 👍 I can see that you have a good understanding of HTML form basics!
- Please check the accessibility rating of your form. Currently it is '89' in Lighthouse.
- I left a few comments for you to review in order to improve code quality and the 'look' of the form.
| <div> | ||
| <label for="color">Color</label> | ||
| <select name="color" id="color" required> | ||
| <option disabled>Color</option> |
There was a problem hiding this comment.
Can you find a way to make a placeholder text appear in the dropdown (i.e. "Color") instead of "White"?
| <footer> | ||
| <!-- change to your name--> | ||
| <h2>By HOMEWORK SOLUTION</h2> | ||
| <h5>By Fida H Ali Zada</h5> |
There was a problem hiding this comment.
You might want to consider using a different HTML element for your name within the footer. Right now, having <h5> in the footer will signal to screen readers that there is something new and important in the footer. In a sense, it breaks the semantic order of the page.
| </legend> | ||
| <div> | ||
| <label for="name">Name</label> | ||
| <input type="text" id="name" name="customer-name" placeholder="name" required> |
There was a problem hiding this comment.
Might want to consider have the placeholder name be 'Name' (uppercase) since the placeholder for email uses uppercase.
| </select> | ||
| </div> | ||
|
|
||
| <h2>T-shirt Size</h2> |
There was a problem hiding this comment.
Can you find a way to style the radio buttons so they are not misaligned?
Here is an example of well-aligned vertical radio buttons:
jenny-alexander
added
Reviewed
2 participants
Self checklist
Created an HTML form to collect the name and email address of the customers along with their size and color preferences. The form is validated with built-in HTML validation.