London | 26-ITP-Sep | Mahir Shah | Sprint 1 | Form Controls - #1495
MahirShah300 wants to merge 5 commits into
Conversation
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
MahirShah300
added
the
Needs Review
There was a problem hiding this comment.
Nearly there. The validation itself is well done: the name pattern accepts accented and non-Latin letters and rejects spaces-only input, the email uses the right input type, and both dropdowns start on a "Please select" option so required can actually do its job. Small, clearly described commits too, and the Netlify Lighthouse audit shows Accessibility at 100.
One thing stops this being Complete: there is no way to submit the form. Open the deploy preview, fill in nothing and try to submit it. What happens, and what element is missing? Until a user can trigger submission, none of the required, pattern or type="email" checks ever run for them.
Add the Needs Review label again once you've pushed and I'll take another look.
| <option value="xxl">Extra Extra Large</option> | ||
| </select> | ||
| </div> | ||
| </form> |
There was a problem hiding this comment.
The form ends here without any button. Try filling it in on the deploy preview and submitting. What happens? Which element would let the user do that, and would the browser then run the validation you've written?
There was a problem hiding this comment.
Nothing happens because I haven't added a button. Adding the button, then when clicking it the browser validates the inputs
There was a problem hiding this comment.
That's it. The button is what lets the browser run the checks you'd written.
| name="name" | ||
| required | ||
| minlength="2" | ||
| pattern="(\p{L}\p{M}*)+(['\- ](\p{L}\p{M}*)+)*" |
There was a problem hiding this comment.
Good pattern. I tried Är, ää, O'Brien, Anne-Marie and 李明 and they all pass, while and A are rejected. One question: what does minlength="2" add that the pattern doesn't already do on its own?
There was a problem hiding this comment.
The pattern accepts single characters and the requirement is at least 2 non space characters, which is why I used the min length
There was a problem hiding this comment.
Good answer. The pattern controls which characters, minlength controls how many. Both are needed.
| <div> | ||
| <label for="colour">Colour</label> | ||
| <select name="colour" id="colour" required> | ||
| <option value="">Please select</option> |
There was a problem hiding this comment.
Starting the dropdown on an empty "Please select" option is the right way to make required work on a select. Well done.
| ['\- ] 1 space ' - character. (\p{L}\p{M}*)+)* then | ||
| 0 or more of any letter again --> | ||
| </div> | ||
| <br /> |
There was a problem hiding this comment.
These <br /> tags are only there to space the fields out. What would you reach for instead of line breaks to control spacing? (Not required for this task.)
There was a problem hiding this comment.
Yes. Margins in CSS, whenever you next style a form.
abdishakoor-dev
added
Reviewed
MahirShah300
added
the
Needs Review
abdishakoor-dev
left a comment
abdishakoor-dev
left a comment
There was a problem hiding this comment.
Submit button in, validation runs, and your answers to the three questions are all right. Marking this Complete.
One small thing for the next time you touch this file: Prettier now flags line 66, the new <br> doesn't match the <br /> style used elsewhere in the file. Running the formatter once more would sort it. Not holding Complete for that.
abdishakoor-dev
added
Complete
2 participants
Learners, PR Template
Self checklist
CYF-1004
Changelist
Added the code to ask and validate name and email. Used regex to validate name. Added dropdown for colour and size.