Manchester | 26-ITP-Sep | Aklilu Mihtsun | Sprint 1 | Form Controls - #1499
AkliluMihtsun wants to merge 7 commits into
Conversation
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
AkliluMihtsun
added
the
Needs Review
tiakavousi
added
Review in progress
|
Nice work! You've made a solid start: semantic structure with header/main/footer, proper fieldset/legend grouping, and every label is correctly tied to its input with for/id. But there is a mix of must-fix issues and polish suggestions. Please add the Needs Review label again once you've pushed. |
tiakavousi
added
Reviewed
| <legend>T-shirt Colour and Size</legend> | ||
| <p> | ||
| <label for="red">Red</label> | ||
| <input type="radio" id="red" name="colour"> |
There was a problem hiding this comment.
Radio buttons have no value attribute. Have you considered what value will be submitted for colour when the user selects for example Red?
| <legend>T-shirt Colour and Size</legend> | ||
| <p> | ||
| <label for="red">Red</label> | ||
| <input type="radio" id="red" name="colour"> |
There was a problem hiding this comment.
Is selecting a colour optional according to the requirements? If not, how could you make sure the user cannot submit the form without choosing one?
| </p> | ||
|
|
||
| </fieldset> | ||
|
|
There was a problem hiding this comment.
where the user can submit the form? Check the requirements and consider adding an appropriate submit control.
| </form> | ||
| </main> | ||
| <footer> | ||
| <!-- change to your name--> |
There was a problem hiding this comment.
this comment can be removed as it is only a reminder for you
| @@ -16,12 +17,57 @@ <h1>Product Pick</h1> | |||
| <!-- write your html here--> | |||
There was a problem hiding this comment.
Probably some tidying up needed here as the comments are meant for you to write the code and then they can be removed
| </p> | ||
| <p> | ||
| <label for="size">Tshirt Size</label> | ||
| <select id="size" name="size"> |
There was a problem hiding this comment.
Right now XS is pre-selected, so a user can submit without consciously choosing a size. How we can avoid this?
| -What is the customer's email? I must make sure the email is valid. Email addresses follow a consistent pattern | ||
| -What colour should this T-shirt be? I must provide 3 options. How will I ensure they do not choose other colours? | ||
| -What size does the customer want? I must provide the following 6 options: XS, S, M, L, XL, XXL --> | ||
| <fieldset> |
There was a problem hiding this comment.
Indentation is inconsistent, please consider using tools like Prettier to auto format your files
| <legend>Customer Information</legend> | ||
| <p> | ||
| <label for="customerName">Customer Name</label> | ||
| <input id="customerName" name="customerName" type="text" pattern=".*\S.*\S.*" required> |
There was a problem hiding this comment.
I can see that the customerName input has validation using required and pattern. How will the user know what is expected before submitting the form? Consider how you could communicate the requirement (at least two non-space characters) to the user so they can provide a valid value on the first attempt and avoid unnecessary round trips.
There was a problem hiding this comment.
Radio button layout is unaddressed
No focus styles : Removing/relying on default focus rings is an accessibility issue for keyboard users. Add a visible focus indicator
select uses margin-left while text inputs use display: block. This is inconsistent please pick one pattern for the whole form so it feels cohesive
Footer is bold but has no spacing or alignment. A little breathing room is nice here , maybe adding some padding and margin
2 participants
Learners, PR Template
Self checklist
Task code
CYF-1004
Changelist
-Used semantic HTML elements to improve the structure and accessibility of the page
and multiple sections-Added a structured customer/product form using
-Added elements to describe each fieldset
-Added required validation to fields that must be completed
-Added a pattern to ensure the customer's name contains at least two non-space characters
-Added styling for the page layout and form
-Checked the page with Lighthouse and achieved an Accessibility score of 100
Questions
-The "for" attribute in labels does it point to the id or name of the input?
-Why does the required attribute does not have value? Can we put value?