London | 26-ITP-Jan | Miriam Jorna | Sprint 2 | Form Controls#1154
London | 26-ITP-Jan | Miriam Jorna | Sprint 2 | Form Controls#1154miriamjorna wants to merge 13 commits intoCodeYourFuture:mainfrom
Conversation
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
miriamjorna
added
📅 Sprint 2
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Can you also take a look at this General Feedback to see if there
is anything you can do to make your PR more robust and ready?
cjyuan
added
Reviewed
miriamjorna
changed the title
…and spaces are no longer valid input). Have put label tags on radio buttons as proposed. Put sizes in a table to make page shorter, so that Submit button doesn't run out of sight.
…dule-Onboarding into tshirt-form-clean
miriamjorna
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 error in your code. Can you fix it?
Form-Controls/index.html
Outdated
| <p> | ||
| <label for="name"> | ||
| Name: <input type="text" id="name" minlength="2" name="user_name" | ||
| pattern="[A-Za-z]{2}" title="At least two letters are needed" required autofocus /> |
There was a problem hiding this comment.
Have you tested this input field to see if it can correctly accept and reject the values as expected?
There was a problem hiding this comment.
Ahhh... I did several times, but clearly not after the last change! Thanks for spotting that
Form-Controls/index.html
Outdated
| <p> | ||
| <label for="email"> | ||
| Email: <input type="email" id="email" name="user_email" required /> | ||
| </label> | ||
| </p> |
There was a problem hiding this comment.
Can you improve the indentation of the code in this file?
There was a problem hiding this comment.
Have changed this.
About line 50:
When I ask VSCode to autoformat it keeps changing my change on line 50 (into a more vertical listing as with Name) back into one line. This suggests I'm doing it wrong? I'm leaving it as autoformatted.
For clarity, the /> is now a >
There was a problem hiding this comment.
The trailing slash / in <input ... /> is not needed in HTML.
Not sure what you meant by "more vertical listing".
Is this your concern?
<input type="email" id="email" name="user_email" required />
is changed to
<input
type="email"
id="email"
name="user_email"
required
>
?
Both formats work.
There was a problem hiding this comment.
Yes, that's what I did, and then VSCode keeps changing it back to one line. I tried the vertical form to find out if that would be what you meant by an improvement on the indentation.
Form-Controls/index.html
Outdated
| <table> | ||
| <tr padding="30px"> |
There was a problem hiding this comment.
It is a bad practice to use <table> for layout purpose. A normal approach would be to use CSS.
However, without CSS, we can still create enough spacing to reach 100 in Lighthouse accessibility score using some elements that can produce space/gap in HTML.
cjyuan
added
Reviewed
…oing colours earlier) - After validation removed a whole lot of empty slashes in /> and replaced with just > - Removed invalid padding code - Responded to comments in review Thank you for your patience
miriamjorna
added
Needs Review
|
Changes look good enough. There are several areas the code can be further improved. Can you share your code to ChatGPT and ask it for possible improvements? I will mark this PR as complete first. |
cjyuan
removed
the
Needs Review
cjyuan
added
the
Complete
|
Hurray and thank you! |
miriamjorna
changed the title
3 participants
London | Jan26ITP | Miriam Jorna | Sprint 2 | T-shirt form
Self checklist
Changelist
Homework for Sprint 2: HTML form. Now in a clean branch.