London | 26-ITP-Sep | Hanna Bohlin | Sprint 1 | Form controls - #1497
London | 26-ITP-Sep | Hanna Bohlin | Sprint 1 | Form controls#1497habohlin wants to merge 33 commits into
Conversation
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
habohlin
added
Needs Review
abdishakoor-dev
left a comment
abdishakoor-dev
left a comment
There was a problem hiding this comment.
All the requirements are met and the form works. Good commits too, small and clearly named.
One thing to sort before I mark this Complete: Prettier fails on both files. In index.html lines 52 to 88 are indented one level too deep, and styles.css uses a different indent width. Run npx prettier --write Form-Controls from the repo root and push.
I've left a few questions inline, none of them blockers. Add the Needs Review label again once you've pushed.
| *I commit often and push regularly to GitHub | ||
| --> | ||
|
|
||
| <fieldset id="customer-info"> |
There was a problem hiding this comment.
Everything from here to line 88 is one indent level deeper than the rest of the form. That's what Prettier is flagging. Running the formatter will fix it.
| </fieldset> | ||
| <fieldset id="tshirt-info"> | ||
| <legend>T-shirt info</legend> | ||
| <p>Choose a colour:</p> |
There was a problem hiding this comment.
A <p> doesn't connect this text to the three radio buttons for a screen reader. You've already used fieldset and legend above for the same job. Would that work here?
| </div> | ||
| <label for="size">Choose a size:</label> | ||
| <select name="size" id="size" required> | ||
| <option label="choose"></option> |
There was a problem hiding this comment.
Works, the value is empty so required kicks in. Why label="choose" rather than putting the text between the tags, like the MDN examples from the prep?
| <!-- | ||
| try writing out the requirements first as comments | ||
| this will also help you fill in your PR message later--> | ||
| <!-- Learning objectives |
There was a problem hiding this comment.
This was a useful checklist while you worked, but it's the README pasted into the page. Does it need to be here now it's all done? Comments in the code are for whoever reads the code later. A checklist like this belongs in the PR description.
| margin: 7px; | ||
| } | ||
|
|
||
| section { |
There was a problem hiding this comment.
There's no <section> in the page any more, you changed it to a div. What is this rule styling?
abdishakoor-dev
added
Reviewed
habohlin
added
Needs Review
abdishakoor-dev
left a comment
abdishakoor-dev
left a comment
There was a problem hiding this comment.
Prettier passes on both files now, the checklist comment is gone and the stylesheet rule matches the element. Marking this Complete.
The two questions I left on the colour heading and the size placeholder option are still worth a think for next time, but nothing more is needed here.
abdishakoor-dev
added
Complete
2 participants
Learners, PR Template
Self checklist
Task code
CYF-1004
Changelist
Create a form allowing users to request a t-shirt. The form asks for their name, email, t-shirt colour and size. The form uses sematntic html for structure, and css for styling.