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

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

Develop #795

wants to merge 10 commits into from

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