Skip to content

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
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