Conversation
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Alaterry8
added
the
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
removed
the
Needs Review
This comment has been minimized.
This comment has been minimized.
Alaterry8
changed the title
This comment has been minimized.
This comment has been minimized.
Alaterry8
changed the title
Alaterry8
added
the
Needs Review
Liam310
added
Review in progress
| this will also help you fill in your PR message later--> | ||
| <form class="form"> | ||
| <label for="name">Name:</label> | ||
| <input type="text" name="name" id="name" minlength="2" required /> |
There was a problem hiding this comment.
minlength is a useful attribute, but it doesn't do exactly what is being asked by the instructions for this task. Have a read of the instructions again for what the requirement is for validating the name input (and read the tip that helps you figure out how to attack it!)
|
|
||
| <label for="colour"> Colour: </label> | ||
| <select name="colour" id="colour" required> | ||
| <option value="select" selected disabled>Select</option> |
There was a problem hiding this comment.
It's a nice user experience to have a placeholder option in a select like you have here, and it's almost there but not quite. It's great that used selected and disabled on this, but at the moment something else isn't quite right because I'm still able to submit the form without changing this drop down at all, which isn't what we want. I should have to choose an actual colour.
Have a look at the MDN documentation for select. What have they done in their example that's different? Why does that matter?
| <br /><br /> | ||
| </fieldset> | ||
|
|
||
| <br /><br /> |
There was a problem hiding this comment.
I can see you've used br a lot here to create space between the different form elements. This is a neat trick, but is actually not what it is supposed to be used for. To quote MDN:
The
<br>element has a single, well-defined purpose — to create a line break in a block of text.
If you are adding spacing between elements on a page, that's exactly what CSS is for. Shortcuts like this can seem useful but they run the risk of causing unexpected confusion for assistive technologies when used out of place!
Note: styling is not required for this task. I'm not going to require you to change anything on this point, but bear in mind for future that you should avoid using br tags for this reason 🙂
|
Hey @Alaterry8! You've showcased a good understanding of some appropriate choices for form elements here along with a good start with validating the inputs. As it stands, there are still a couple of inputs that are considered valid when they shouldn't be. Have a read of comments and see what you think! |
Liam310
added
Reviewed
2 participants
Self checklist
Task ID: CYF-1004
Changelist
I agree to follow the code of conduct for this organisation.