Skip to content

Conversation

@Kacper-Leman
Copy link

@Kacper-Leman Kacper-Leman commented May 22, 2023

Copy link

@DorotaLeniecDev DorotaLeniecDev left a comment

Choose a reason for hiding this comment

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

Fix formatting and indentations, check this checklist https://github.com/mate-academy/layout_html-form/blob/master/checklist.md

When the code is more readable after fixes we will continue with review

src/index.html Outdated
Comment on lines 17 to 19
<div>
Surname: <input type="text" name="Surname:">
</div>

Choose a reason for hiding this comment

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

Add proper labels
Fix formatting
Fix indentations

Suggested change
<div>
Surname: <input type="text" name="Surname:">
</div>
<div class="form__section_field">
<label for="surname">Surname: </label>
<input
type="text"
name="surname"
id="surname"
autocomplete="off"
>
</div>

Apply this pattern to all other fields
Remember to add spaces between sibling elements for better readability

Copy link
Author

Choose a reason for hiding this comment

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

Okey, I make fixes please to check again :)

Copy link

@DorotaLeniecDev DorotaLeniecDev left a comment

Choose a reason for hiding this comment

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

Few more changes needed

src/index.html Outdated

<input
type="date"
name="date-birth"

Choose a reason for hiding this comment

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

Name attibute should be camelCase

Suggested change
name="date-birth"
name="dateBirth"

src/index.html Outdated
Comment on lines 77 to 79



Choose a reason for hiding this comment

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

Only one line space between siblings is needed

src/index.html Outdated
Comment on lines 109 to 111



Choose a reason for hiding this comment

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

Same here

src/index.html Outdated
Comment on lines 196 to 202
<textarea
name="comment"
id="comment"
cols="20"
rows="2"
autocomplete="off">
</textarea>

Choose a reason for hiding this comment

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

image Fix formatting

src/index.html Outdated
Comment on lines 208 to 212
<select
name="recommend"
id="recommend"
autocomplete="off">

Choose a reason for hiding this comment

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

Fix formatting

Suggested change
<select
name="recommend"
id="recommend"
autocomplete="off">
<select
name="recommend"
id="recommend"
autocomplete="off"
>

src/index.html Outdated
Comment on lines 113 to 114
<legend>An interesting fact about you!</legend>
<div>

Choose a reason for hiding this comment

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

Add empty line between siblings

src/index.html Outdated
Comment on lines 81 to 82
<legend>Registration</legend>
<div>

Choose a reason for hiding this comment

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

Add empty line - double check if there are empty lines between siblings elements, do not add empty lines between parent and child

src/index.html Outdated

<input
type="radio"
name="choice"

Choose a reason for hiding this comment

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

Lets make this name more descriptive so it is easier to use (in scenartio when form is being send)

Suggested change
name="choice"
name="lovesCats"

src/index.html Outdated
<label for="yes">Yes</label>

<input type="radio"
name="choice"

Choose a reason for hiding this comment

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

Same here

Suggested change
name="choice"
name="lovesCats"

src/index.html Outdated
<form action="https://mate-academy-form-lesson.herokuapp.com/create-application" class="form">
<fieldset>
<legend>Personal information</legend>
<div>

Choose a reason for hiding this comment

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

Be consistant and add class to all form fields wrappers. You can then specify the margins using pseudo selectors in css

Copy link
Author

Choose a reason for hiding this comment

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

All tips done I hope now its good.

Copy link

@DorotaLeniecDev DorotaLeniecDev left a comment

Choose a reason for hiding this comment

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

Few more for perfection :)

src/index.html Outdated

<input
type="color"
name="colour"

Choose a reason for hiding this comment

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

fix typo

Choose a reason for hiding this comment

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

Should be

Suggested change
name="colour"
name="color"

src/index.html Outdated
autocomplete="off"
multiple
>

Choose a reason for hiding this comment

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

Remove space

src/index.html Outdated
>
</div>

<div>

Choose a reason for hiding this comment

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

In my opinion best way is to add form-field class to all divs and in css you can remove margin-bottom in :last-child selector. This is more consistant. It is a separation of duites, that styles will determin how the markup looks.

src/index.html Outdated
Comment on lines 126 to 131
<input type="radio"
name="lovesCats"
id="no"
autocomplete="off"
value="No"
>

Choose a reason for hiding this comment

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

Fix formatting

Suggested change
<input type="radio"
name="lovesCats"
id="no"
autocomplete="off"
value="No"
>
<input
type="radio"
name="lovesCats"
id="no"
autocomplete="off"
value="No"
>

src/index.html Outdated
id="recommend"
autocomplete="off"
>

Choose a reason for hiding this comment

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

remove space

Copy link

@DorotaLeniecDev DorotaLeniecDev left a comment

Choose a reason for hiding this comment

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

Good job :)

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