London | 26-ITP-SEP | Abdennour Hachemi | Sprint 1 | form-controls - #1503
AbdennourHachemi wants to merge 9 commits into
Conversation
❌ Deploy Preview for cyf-onboarding-module failed. Why did it fail? →
|
AbdennourHachemi
added
the
Needs Review
There was a problem hiding this comment.
Good start, and the coloured squares behind the colour options are a nice touch. Things to sort before it can be marked Complete:
- No submit button, so the form can't be submitted and none of the validation ever runs. See line 90.
- An empty name goes through. The pattern is right, so something else is missing. See line 20.
- The form goes through without a size. See line 55.
- A stray full stop on line 50 shows up on the page, and the HTML validator from the README checklist (https://validator.w3.org/) reports one error, on line 39.
- Formatting. Prettier fails on both files. Run
npx prettier --write Form-Controlsfrom the repo root and push.
Add the Needs Review label again once you've pushed.
| </div> | ||
| </div> | ||
| </div> | ||
| </form> |
There was a problem hiding this comment.
The form closes here with no submit button. Open the deploy preview and try to send the form. What's missing, and what does the browser do with required and pattern until it's there?
| <main> | ||
| <form> | ||
| <div id="fn"><label for="fname">Please Enter your First name:</label> | ||
| <input type="text" id="fname" name="fname" pattern=".*\S.*\S.*"> <br> |
There was a problem hiding this comment.
The pattern attribute is right and does what the README asks. Once you've added a submit button, leave the name empty and submit. It goes through. What have you put on the email field on line 26 that this field doesn't have? Also, the task asks for one name field, not first and last.
| <div class="sizeOption"> | ||
| <label for="xs">XS</label> | ||
| <div> | ||
| <input type="radio" name="size" value="xs" id="xs"> |
There was a problem hiding this comment.
Pick no size and submit: the form goes through. You've already solved this for colour on line 45. What's different about these six?
| <input type="email" id="email" name="email" required><br> | ||
| </div> | ||
| <div class="colorSelection"> | ||
| <label>Please pick a color:</label> <br> |
There was a problem hiding this comment.
This label isn't attached to any control, it's just acting as a heading for the group. Which element does the MDN page from the prep use to group a set of radio buttons and give them a caption? Same for line 50.
| </div> | ||
| </div> | ||
| <div id="sizediv"> | ||
| <label>Please select the t-shirt size:</label> <br> . |
There was a problem hiding this comment.
There's a stray . after the <br> here and it shows on the page.
| <div class="colorOption"> | ||
| <label for="greenOption">Green</label> | ||
| <div class="square" id="greenOptions"> | ||
| <input type="radio" name="color" id="greenOption"value="green"> |
There was a problem hiding this comment.
Missing space between id="greenOption" and value="green". This is the one error the HTML validator (https://validator.w3.org/, from the README checklist) reports. The formatter will fix it.
abdishakoor-dev
added
Reviewed
2 participants
Self checklist
Task code
CYF-1004
A solution to the T-shirt order form using HTML , i have added an external css file , had to close to first pull request since there were some accessiblity issues, I was confused on how to do the PR , but the instructions and searching the net was helpful.