NW | 25-ITP-Sep | TzeMing Ho | Sprint 2 | Form Controls #738
Conversation
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. |
TzeMingHo
added
📅 Sprint 2
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. |
1 similar comment
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. |
LonMcGregor
added
the
Review in progress
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Good start on this sprint's tasks, I have spotted a few areas where you could improve code further
Remember to update the default PR text when submitting on github. If you have no questions you can remove that section
Did you mean to include the wireframe files? We try to keep pull requests specific to just the files in the feature we are changing - for this, everything within the Form-Controls directory.
Form-Controls/README.md
Outdated
There was a problem hiding this comment.
Hi Lon,
Thank you for your reminder. I have deleted Wireframe/index.html, Wireframe/style.css, and Form-Controls/README.md.
There was a problem hiding this comment.
If you look at the "changed files" section on this Pull Request, you will see that these files are all listed as changed. This means if this PR were merged, those files would be deleted for everyone from all new branches. What we want to achieve is for the "changed files" area to only show changes. Instead of deleting files that you don't want changed entirely, you need to revert them to their original state before modification. Can you figure out how to do that?
There was a problem hiding this comment.
Hi Lon,
Thank you for the clarification. Now I understand where to look for file changes. It was a valuable lesson to learn about how to restore files from the main, and I feel more confident about not messing up the original state.
Form-Controls/index.html
Outdated
| id="name" | ||
| name="name" | ||
| minlength="2" | ||
| pattern="^[A-Za-z]{2,}$" |
There was a problem hiding this comment.
Good to see you using a regular expression here. This might be a bit restrictive, however. Think about the kind of names people might use. What if they put a firstname and surname? What if their name has non-english characters?
There was a problem hiding this comment.
Hi Lon,
Thank you for your suggestion. I was not thoughtful enough to think about users may input both their first name and surname, or even non-English characters. The pattern for name input is deleted in [f91b77e].
There was a problem hiding this comment.
Validation issues like that are a common pitfall - good fix
Form-Controls/index.html
Outdated
| type="email" | ||
| id="email" | ||
| name="email" | ||
| pattern="^[A-Za-z0-9._]+@[A-Za-z0-9]+\.[a-z]{2,}(\.[a-z]{2,})?$" |
There was a problem hiding this comment.
Do you need to include a pattern for this input type?
There was a problem hiding this comment.
Hi Lon,
A pattern for email is not required in the exercise. However, I read the MDN suggestion to add a pattern for email validation. So, I included it to practice the concept. The pattern has been removed in [f91b77e].
There was a problem hiding this comment.
Most browsers now include the same pattern built-in, so you don't need to specify it. Thanks for fixing this
LonMcGregor
added
Reviewed
|
I've left some additional comments. Remember if you are done making changes and want some feedback to add the "needs review" label to this PR. |
TzeMingHo
added
the
Needs Review
|
Great work on this task - you are done with this PR now |
LonMcGregor
added
Complete
3 participants
Learners, PR Template
Self checklist
Changelist
I have edited the HTML file with a fieldset, accepting customers' inputs for their names, emails, the colour of the T-shirt, and the size of the T-shirt, while adding a legend and a label for each input.