London | 26-ITP-Sept | Fung Nin Lee | Sprint 1 | Forms Control - #1484
leerogerfn wants to merge 22 commits into
Conversation
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
hey-hammad
left a comment
There was a problem hiding this comment.
Good work overall! The form is clear and covers the main requirements. A few small improvements to structure, validation, and accessibility would make it stronger.
| <h1>Product Pick</h1> | ||
| <fieldset> | ||
| <legend>Product Pick</legend> | ||
| </header> |
There was a problem hiding this comment.
Can you please review the header tag, there is no opening <header> tag.
| <input type="text" id="name" name="name" pattern=".*\S.*\S.*"> | ||
| <div> | ||
| <label for="email">Email</label> | ||
| <input type="email" id="email" name="email"> |
There was a problem hiding this comment.
The task says all fields are required. How will the form prevent submission if the user leaves the name, email, colour, or size blank?
| </main> | ||
| </fieldset> | ||
|
|
||
| <button type="submit">submit</button> |
There was a problem hiding this comment.
The submit button currently doesn't do anything, will moving to another place fix this?
There was a problem hiding this comment.
Hi Hammad, thank you for reviews and comments. It seems the requirement did not include submitting information to sowhere. Could you elaborate more what I have to do on this point?
There was a problem hiding this comment.
The button is in the right place now, inside the form. The task says not to write a form action, so it doesn't need to send the data anywhere. Its job is to make the browser run the validation when it's clicked. Nothing more to do on this one.
| <label for="email">Email</label> | ||
| <input type="email" id="email" name="email"> | ||
| </div> | ||
| <p></p> |
There was a problem hiding this comment.
is there a reason for adding empty <p> tag here?
| <body> | ||
| <header> | ||
| <h1>Product Pick</h1> | ||
| <fieldset> |
There was a problem hiding this comment.
Please review the fieldset tags, think about the correct placement when building a form, more on this in the link below. Thanks
https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/fieldset
…ed the requirement. And the pull down menu is required to select an option.
abdishakoor-dev
left a comment
There was a problem hiding this comment.
Picking this up from Hammad's review. The validation is now right: name, email, colour and size are all required and the name pattern works. Good.
Things to sort before it can be marked Complete:
- The
<header>has ended up inside<head>(line 7).<head>is for things like<title>and<meta>. Everything the user sees goes in<body>. The starter file on main had the header in the right place, compare against it. - The
<div>opened on line 24 is never closed. The HTML validator from the README checklist (https://validator.w3.org/) gives up at that point with 10 errors, and Prettier can't parse the file at all. Fix the structure, then run the validator again until it's clean. requiredon line 43 is on an<option>, where it isn't allowed. You already have it on the<select>, which is where it works.- "Color" on line 26 is loose text. Hammad pointed you at fieldset for this and it was removed rather than moved. Which element gives a group of radio buttons a caption?
- The footer still has HOMEWORK SOLUTION in it, line 61.
- Formatting. Once the structure is fixed, run
npx prettier --write Form-Controlsfrom the repo root. The indentation is all over the place at the moment.
Add the Needs Review label again once you've pushed.
| <meta charset="utf-8" /> | ||
| <meta http-equiv="X-UA-Compatible" content="IE=edge" /> | ||
| <title>My form exercise</title> | ||
| <header><h1>Product Pick</h1></header> |
There was a problem hiding this comment.
This is inside <head>. Which part of the page does <head> describe, and where should something the user sees go? The starter file had this right.
| <input type="email" id="email" name="email" required> | ||
| </div> | ||
|
|
||
| <div> |
There was a problem hiding this comment.
This <div> is never closed. Count the opening and closing divs from here to line 51. That's why the validator stops and Prettier can't format the file.
There was a problem hiding this comment.
Fixed, and the validator is clean now.
| <div> | ||
| <label for="size">Size</label> | ||
| <select name="size" id="size" required> | ||
| <option disabled hidden selected required value="">Select One</option> |
There was a problem hiding this comment.
required isn't an attribute an <option> can have. The one on the <select> on line 42 is the one doing the work, so this can go.
|
|
||
| <div> | ||
| <br> | ||
| Color |
There was a problem hiding this comment.
Loose text as a heading for the radios. Hammad's fieldset link was about this. What does the MDN page from the prep on structuring a form use to group radio buttons and caption them?
There was a problem hiding this comment.
That's the one. Fieldset and legend, done.
| <option value="xxl">XXL</option> | ||
| </select> | ||
| </div> | ||
| <p></p> |
There was a problem hiding this comment.
Hammad asked about the empty <p> and there's still one here. What is it for?
|
|
||
|
|
||
| <footer> | ||
| <p>By Fung Nin Lee (Roger) HOMEWORK SOLUTION</p> |
There was a problem hiding this comment.
HOMEWORK SOLUTION should have gone when you put your name in.
There was a problem hiding this comment.
Hi Abdi. Thank you for your comments. Could you feel free to check if it is OK now?
There was a problem hiding this comment.
Yes, all the structure points are done. One thing left, formatting, see the main comment.
There was a problem hiding this comment.
All six structure points are sorted: header in the right place, the div closed, required off the option, fieldset and legend on the colour radios, the empty <p> gone and your name in the footer. The validator is clean and every field is validated the way the task asks. Good work getting through that list.
One thing left before I mark this Complete: the formatting check. "My code is consistently formatted" is on the README checklist, and the tool that does it for you is called Prettier. It rearranges spacing and indentation to one agreed style, so that your code is easy to read and so that a reviewer only sees the changes you meant to make, not stray spaces and tabs. At the moment index.html fails that check; look at how far the indentation wanders between lines 14 and 35.
Prettier comes with the CYF extension pack you were asked to install during onboarding. If you're not sure you have it, open VS Code, go to Extensions, and search for CodeYourFuture Extension Pack; install it if it isn't there: https://marketplace.visualstudio.com/items?itemName=CodeYourFuture.cyf-extension-pack
Then open index.html, right click in the editor, choose Format Document, and pick Prettier if VS Code asks which formatter to use. Save, commit the changes it makes, push, and add the Needs Review label again. To make this happen automatically every time you save, follow the format on save steps here: https://github.com/CodeYourFuture/Module-JavaScript-Fundamentals/blob/main/practical_guide.md
Hi @abdishakoor-dev , I just completed formatting check. Please confirm if it is good to complete. Thank you. |

Learners, PR Template
Self checklist
Task code
CYF-1004
Changelist