Sheffield | 26-ITP-Jan | Seti Mussa | Sprint 2 | Form-Control #1160
Sheffield | 26-ITP-Jan | Seti Mussa | Sprint 2 | Form-Control #1160Seti-Jemal wants to merge 8 commits intoCodeYourFuture:mainfrom
Conversation
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Seti-Jemal
added
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
- According to https://validator.w3.org/, there is an in your code. Can you fix them?
There was a problem hiding this comment.
What is this file for? If it is not a required file in implementing the T-shirt order form, you should delete it to keep the PR branch clean.
There was a problem hiding this comment.
When I deleted the file , the form control stopped working as well.
Form-Controls/index.html
Outdated
| <fieldset> | ||
| <legend>Please select a size:</legend> | ||
| <label for="size">Size</label> | ||
| <select name="size" id="size" required> | ||
| <option value="">Select size</option> | ||
| <option value="xs">XS</option> | ||
| <option value="s">S</option> | ||
| <option value="m">M</option> | ||
| <option value="l">L</option> | ||
| <option value="xl">XL</option> | ||
| <option value="xxl">XXL</option> | ||
| </select> | ||
| </fieldset> |
There was a problem hiding this comment.
Can you improve the indentation of the code on this file?
Form-Controls/index.html
Outdated
| <form action="results.html" method="GET" autocomplete="on"> | ||
| <div> | ||
| <label for="name">Name</label> | ||
| <input type="text" name="name" id="name" minlength="2" required autocomplete="name"> |
There was a problem hiding this comment.
Currently a user can enter a name consisting of only space characters (e.g., " "). Can you enforce a stricter validation rule using the pattern attribute to disallow any name that contains only space characters?
cjyuan
added
Reviewed
Hi Cjyuna , I fixed it . |
|
If you have made any changes, you need to push them to GitHub too. (Currently I don't see any new commits on this PR branch.) I am guessing you are still working on your PR. If it is ready to be re-reviewed, don't forget to add the "Needs Review" label. |
Form-Controls/index.html
Outdated
| </form> | ||
| <main> | ||
| <h1>Order Form</h1> | ||
| <form action="results.html" method="GET" autocomplete="on"> |
There was a problem hiding this comment.
For testing, you can set the action attribute to "#" so that the form data is sent to the same URL.
There is no need to prepare a separate file to accept the submitted data. This way, you don't need to introduce result.html in this repo.
Note: The spec actually says, Do not write a form action for this project.
Form-Controls/index.html
Outdated
| <form action="results.html" method="GET" autocomplete="on"> | ||
| <div> | ||
| <label for="name">Name</label> | ||
| <input type="text" name="name" id="name" minlength="2" required pattern=".*/S*." title="Name must contain at least one non-space character" autocomplete="name"> |
There was a problem hiding this comment.
The pattern value is not quite correct. Now the input field would not accept name like "John".
Form-Controls/index.html
Outdated
| <fieldset> | ||
| <legend>Please select a color:</legend> | ||
| <div> | ||
| <input type="radio" id="black" name="color" value="Black" required> | ||
| <label for ="black"> Black</label> | ||
| </div> | ||
| <br> | ||
| <div> | ||
| <input type="radio" id="grey" name="color" value="Grey"> | ||
| <label for ="grey"> Grey</label> | ||
| <br> | ||
| </div> | ||
| <div> | ||
| <br> | ||
| <input type="radio" id="red" name="color" value="Red"> | ||
| <label for ="red"> Red</label> |
There was a problem hiding this comment.
Can you improve the indentation of the code on this file?
VSCode's "Format Document" feature can help us format our code for better readability and consistency.
To use the feature, right-click inside the code editor and select the option.
Seti-Jemal
added
the
Needs Review
|
Changes look good. Well done. Do you still need I will mark this PR as "Complete" first. |
cjyuan
added
Complete
2 participants
Learners, PR Template
Self checklist
Changelist
I included three colour options and six size to give user more choices. I also added name and email filed so customer can enter their information.