Manchester | 26-ITP-Jan | Ahmed Elmahmoudi | Sprint 2 | Form-Controls#1177
Manchester | 26-ITP-Jan | Ahmed Elmahmoudi | Sprint 2 | Form-Controls#1177Alaterry8 wants to merge 15 commits intoCodeYourFuture:mainfrom
Conversation
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Form-Controls/index.html
Outdated
|
|
||
| <br> <br> | ||
| <label for="colour"> T-shirt colour:</label> | ||
| <select id="colour" name="T-shirt colour"> |
There was a problem hiding this comment.
Note, we must require the user to actively select colour. Therefore initial option should be empty and we must demand the choise.
There was a problem hiding this comment.
I fixed this by adding an empty option value, and made it required to choose a colour. Thanks!
There was a problem hiding this comment.
Thanks for the feedback, I added an indication text.
Form-Controls/index.html
Outdated
| try writing out the requirements first as comments | ||
| this will also help you fill in your PR message later--> | ||
| <label for="name"> Name: </label> | ||
| <input type="text" id="name" name="Name" minlength="2" required> |
There was a problem hiding this comment.
The convention for naming elements (i.e' name) is lowercase
There was a problem hiding this comment.
so the only thing that can be capitalised is what goes between the label tags?
There was a problem hiding this comment.
They serve different purposes. The text in the label is what users see on their browser. The value of name is intended to be consumed by a server-side script.
Can you use an AI tool to find out why it is better to express value of names in lowercase?
There was a problem hiding this comment.
Yes, I understand now. Thanks.
Form-Controls/index.html
Outdated
|
|
||
| <br> <br> | ||
| <label for="colour"> T-shirt colour:</label> | ||
| <select id="colour" name="T-shirt colour"> |
There was a problem hiding this comment.
the convention for name is to avoid usage spaces. Use underscore or dash instead
There was a problem hiding this comment.
Noted! fixed it. Thanks for all the feedback ;D
There was a problem hiding this comment.
Why use t-shirt_colour for one input and just size (without t-shirt) for the other input?
Either format is acceptable, but it is better to keep the format consistent.
The same also applies to the labels.
There was a problem hiding this comment.
@cjyuan That makes sense, I made the appropriate changes. Thanks for the pointers!
There was a problem hiding this comment.
I was referring to name="t-shirt_colour" on line 27 and name="size" on the radio buttons.
There was a problem hiding this comment.
@cjyuan Yea I understand the issue now, I changed it so it's more consistent
|
Hi @cjyuan @SlideGauge can you kindly review my changes, Thanks! |
|
@Alaterry8 You have done a good job in addressing all of the reviewer's comments. I just have some more suggestions. |
|
@cjyuan @SlideGauge Thanks for all the feedback, I made the suggested changes. |
|
Can you address this #1177 (comment)? Other changes look good. |
|
@cjyuan I made the changes, thanks for the feedback! |
|
Can you push your changes to GitHub? |
|
@cjyuan I did |
|
No new commits made on this branch since yesterday. Did you made your changes on a different branch? If you have made commit on your computer, you need to pusn/sync them to GitHub. |
|
@cjyuan it should be synced now |
|
@cjyuan How do I know that it is synced, where do the changes show on github? |
|
Changes look good. Note: To check if your latest commits were sync, you can check the commits history made on the branch. |


Self checklist
Within a form, I added a name field with minimum 2 characters length requirement. I added an email field. I added a set of 3 colours to select from. I added 6 radio options for Size options.