-
-
Notifications
You must be signed in to change notification settings - Fork 409
London | 26-ITP-Jan | Shuheda Begum | Sprint 2 | FORM-Control #1021
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…dded validation on both names and emails.
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
I added required> to name, email, select colour and select size.
jenny-alexander
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @codebyshay - good work. I left a few comments within the code.
Also, can you check the Lighthouse accessibility score for your form page? I see it as 89 and the exercise requirements say it should be 100.
Form-Controls/index.html
Outdated
| <button type="submit">Submit</button> | ||
| </form> | ||
|
|
||
| <script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you review the exercise instructions? I believe there shouldn't be any javascript in the file.
Form-Controls/index.html
Outdated
| </script> | ||
|
|
||
| <footer> | ||
| <h2>By Shuheda Begum</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The <h2> tag in the footer suggests a major section heading. Footer typically has smaller headings or <p> tags.
Footer changed from <h2> to <p. Block and padding added around the <label tag.
|
Hi, I've amended footer and the <label. This was the cause behind the 89% score on lighthouse. Lighthouse accessibility score is now 100%. Will change the code to eliminate JavaScript tomorrow. |
Form-Controls/index.html
Outdated
| <input type="radio" name="size" value="XS" required> XS | ||
| </label> | ||
|
|
||
| <label style="display:block; padding:12px 0;"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed this in my prior review -> Ensure that you don't add CSS to the file, as stated in the instructions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All done. JavaScript and CSS removed. Lighthouse score 100%. Thank you.
|
@codebyshay - Your form works well. Thanks for making the suggested corrections. |
|
Was that part of the requirements? I don't remember seeing anything about validation via W3? |
|
Hi @codebyshay Even though running W3 validator checks isn't explicitly stated in the CYF requirements, writing valid HTML is a baseline expectation for any web development work. It's a good habit to run your markup through W3 validator at this stage of your development journey. |
|
I appreciate it's good practice, but I believe I have fulfilled the criteria for this part of the course. |
|
@codebyshay Your form implementation looks good. While it's not explicitly stated in the spec, things like "valid HTML" are usually assumed rather than written into requirements in a real job. CYF asks mentors to hold trainees to a high standard in order to better prepare them for the real world. It should be an easy fix. Can you try? |
|
Thank you @cjyuan I am a complete novice when it comes to coding, so I am not familiar with expectations or common practices amongst coders. I am however, trying hard to learn. I appreciate the feedback provided by yourself and @jenny-alexander. I have attempted it to make the code valid, hopefully all is in order now. |

Learners, PR Template
Self checklist
Changelist
I have created a form that collects customer t-shirt orders. I have added validation on both names and emails.
Questions
No Questions