London | 26-ITP-Sep | Bartosz Kawiak | Sprint 1 | Form Controls - #1493
bartoszkawiak wants to merge 4 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.
bartoszkawiak
added
the
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
removed
the
Needs Review
bartoszkawiak
changed the title
bartoszkawiak
added
the
Needs Review
bartoszkawiak
changed the title
bartoszkawiak
changed the title
bartoszkawiak
changed the title
There was a problem hiding this comment.
Most of this is in place. Name, email and size validation all work, there's a submit button, and the comment above each section saying which requirement it covers is a good habit.
Things to sort before it can be marked Complete:
- On the deploy preview, fill in everything else, leave the colour on "Choose here" and submit. It goes through. This is also the one error the HTML validator from the README checklist (https://validator.w3.org/) reports, on line 28. See the comment there.
- The stylesheet doesn't load on the deploy preview. Open it and the page is unstyled. See line 9.
- Formatting. Prettier fails on both files. Run
npx prettier --write Form-Controlsfrom the repo root and push.
Add the Needs Review label again once you've pushed.
| <!--Color 3 options with select NO multiselect--> | ||
| <label for="colorOptions">Color</label> | ||
| <select id="colorOptions" name="color" required> | ||
| <option hidden>Choose here</option> |
There was a problem hiding this comment.
This option has no value attribute, so its value is its text, "Choose here". That's a non-empty value, so the browser treats it as a real choice and required is satisfied before the user does anything. Read the validator's message for this line. What does it say the placeholder option needs?
There was a problem hiding this comment.
Fixed. Empty value, so the browser insists on a choice now.
| <meta name="viewport" content="width=device-width, initial-scale=1" /> | ||
| <meta name="description" content="T-shirt order form, pick your product." > | ||
| <meta name="viewport" content="width=device-width, initial-scale=1" > | ||
| <link rel="stylesheet" href="/styles.css"> |
There was a problem hiding this comment.
/styles.css with a leading slash means "from the root of the site". On the deploy preview this page lives at /Form-Controls/, so the browser asks for a file that isn't there and gets a 404. How would you write the path relative to this HTML file?
There was a problem hiding this comment.
Loads on the preview now. Good.
| </select> | ||
|
|
||
| <!--SIZE 6 options XS,S,M,L,XL,XXL--> | ||
| <p>Choose Size:</p> |
There was a problem hiding this comment.
A <p> as the heading for the size radios doesn't connect the text to the buttons for a screen reader. You've used fieldset and legend for the form as a whole. Would a second one work here?
There was a problem hiding this comment.
Nested fieldset with its own legend, that's the one.
| } | ||
|
|
||
| select:focus{ | ||
| /* scale: 1.1; */ |
There was a problem hiding this comment.
Commented-out code, here and on line 56. The style guide asks for these to be removed rather than left in. Git keeps the history if you want it back.
abdishakoor-dev
added
Reviewed
bartoszkawiak
added
the
Needs Review
abdishakoor-dev
left a comment
abdishakoor-dev
left a comment
There was a problem hiding this comment.
All three sorted, and you did the two optional ones as well. That's everything. Complete.
abdishakoor-dev
added
Complete
2 participants
Learners, PR Template
Self checklist
Task code
CYF-1004
Changelist
Added a responsive T-shirt order form using HTML and CSS, It includes validation for email and name, required color dropdown menu and size selection, also scored 100 on a lighthouse and checked HTML semantics using validator.