Skip to content
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

Develop #796

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Develop #796

wants to merge 3 commits into from

Conversation

natispatis
Copy link

darokrk

This comment was marked as outdated.

Copy link

@darokrk darokrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout looks good, but code needs some improvements :)

src/index.html Outdated
<fieldset class="form__fieldset form-fieldset">
<legend class="form-fieldset__title">An interesting fact about you!</legend>
<div class="field">
<label for="surname">Do you love cats? </label>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not a surname, think this should be rather cats, names of the inputs below also should be changed :)

src/index.html Outdated
</div>
</fieldset>
</form>
<button type="submit">Submit</button>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move button inside form to make it functional

src/index.html Outdated
>
</div>
<div class="field">
<label for="name">Password: </label>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an 'id' attribute with the value 'password' to match the 'for' attribute of the label.

src/index.html Outdated
<fieldset class="form__fieldset">
<legend class="form-fieldset__title">Registration</legend>
<div class="field">
<label for="surname">E-mail: </label>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an 'id' attribute with the value 'email' to match the 'for' attribute of the label.

src/index.html Outdated
<label for="name">Name: </label>
<input type="text"
name="firstname"
minlength="25"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'minlength' attribute value should be reasonable, reduce it to a smaller value (e.g. 2). we dont need that long name value here

<fieldset class="form__fieldset form-fieldset">
<legend class="form-fieldset__title">Personal information</legend>
<div class="field">
<label for="surname">Surname: </label>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Every field should have label, which focuses input on label click", your labels after click not focusing inputs.
Please add an id with the same value as you have in label for attribute and then it should work :)
Do that for the rest form elements also.

<legend class="form-fieldset__title">Personal information</legend>
<div class="field">
<label for="surname">Surname: </label>
<input type="text"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an 'id' attribute with the value 'surname' to match the 'for' attribute of the label.

readme.md Outdated
@@ -1,7 +1,7 @@
# HTML form
Replace `<your_account>` with your Github username and copy the links to Pull Request description:
- [DEMO LINK](https://<your_account>.github.io/layout_html-form/)
- [TEST REPORT LINK](https://<your_account>.github.io/layout_html-form/report/html_report/)
- [DEMO LINK](https://<natispatis>.github.io/layout_html-form/)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think here you should remove brackets <> otherwise links will not work correctly, same for testing

src/index.html Outdated
<input type="color" name="color">
</div>
<div class="field">
<label for="name">What time do you go to bed?</label>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think it's not a label for name, please correct labels for tags in whole file to points correct inputs :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants