Skip to content

Conversation

@Patryk-Buczkowski
Copy link

Copy link

@Radoslaw-Czerniawski Radoslaw-Czerniawski left a comment

Choose a reason for hiding this comment

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

Adjust according to my comments and the checklist

src/index.html Outdated
method="post"
action="https://mate-academy-form-lesson.herokuapp.com/create-application"
>

Choose a reason for hiding this comment

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

There shouldn't be empty line between parent and first child

Choose a reason for hiding this comment

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

You should have empty lines only between siblings on the same level, not between parent and child, here's an example

<section>
  <div>
    <span>First Child of Div</span>

    <span>Second Child of Div</span>
  <div>
</section>

Choose a reason for hiding this comment

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

An exemption to that rule would be an ul

<ul>
  <li></li>
  <li></li>
  <li></li>
  <li></li>
</ul>

Choose a reason for hiding this comment

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

In the rest of your markup doesn't adhere to these rules so adjust accordingly, also take a look at the checklist and also adhere to it's rules

src/index.html Outdated
</head>
<body>
<h1>HTML Form</h1>
<script type="text/javascript" src="./main.js"></script>

Choose a reason for hiding this comment

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

This script should be at the bottom of the markup to work move it below your form


<button type="submit">Submit</button>
</form>

Choose a reason for hiding this comment

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

Remove those 2 empty lines

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 minor changes needed :)

src/index.html Outdated
Comment on lines 22 to 32
<label for="surname">Surname:</label>

<input
id="surname"
type="text"
name="surname"
autocomplete="off"
class="input"
>

<br>

Choose a reason for hiding this comment

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

Remove
tags, you can achive this by grouping form-fiels into block element like this:

Suggested change
<label for="surname">Surname:</label>
<input
id="surname"
type="text"
name="surname"
autocomplete="off"
class="input"
>
<br>
<div class="form-field">
<label for="name">Name:</label>
<input
type="text"
name="name"
id="name"
autocomplete="off"
required
>
</div>

When you wrap label and input into div, it will take 100% width and make newline automatically for next elements. Wrapp all fields like this and remove
:)

src/style.css Outdated
Comment on lines 1 to 4
.input {
margin-bottom: 10px;
display: inline-block;
}

Choose a reason for hiding this comment

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

And you can easly style it like this:

Suggested change
.input {
margin-bottom: 10px;
display: inline-block;
}
.form-field {
margin-bottom: 10px;
}

src/index.html Outdated
Comment on lines 159 to 160
<label for="car" class="input">
What are your favorite brand of cars?

Choose a reason for hiding this comment

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

Close label tag here:

Suggested change
<label for="car" class="input">
What are your favorite brand of cars?
<label for="car" class="input">
What are your favorite brand of cars?
</label>

src/index.html Outdated
Comment on lines 161 to 165
<select
id="car"
name="carBrand"
multiple
>

Choose a reason for hiding this comment

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

Fix formating:

Suggested change
<select
id="car"
name="carBrand"
multiple
>
<select
id="car"
name="carBrand"
multiple
>

src/index.html Outdated
Comment on lines 189 to 190
<label for="comments" class="input">
Comments:

Choose a reason for hiding this comment

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

Suggested change
<label for="comments" class="input">
Comments:
<label for="comments" class="input">
Comments:
</label>

src/index.html Outdated
Comment on lines 199 to 203
<label for="recommend" >Would you recommend us?</label>
<select
id="recommend"
name="recommend"
>

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

Suggested change
<label for="recommend" >Would you recommend us?</label>
<select
id="recommend"
name="recommend"
>
<label for="recommend" >Would you recommend us?</label>
<select
id="recommend"
name="recommend"
>

Comment on lines 207 to 208
</fieldset>
<button type="submit">Submit</button>

Choose a reason for hiding this comment

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

Suggested change
</fieldset>
<button type="submit">Submit</button>
</fieldset>
<button type="submit">Submit</button>

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.

Looks nice 🥇 ! I give approve but please correct the indentation in the code according to the example given :)

<fieldset class="fieldset">
<legend>Personal information</legend>
<div class="form-field">

Copy link

Choose a reason for hiding this comment

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

Like Radek mentioned before there should not be an empty line.

"You should have empty lines only between siblings on the same level, not between parent and child, here's an example"

<section>
  <div>
    <span>First Child of Div</span>

    <span>Second Child of Div</span>
  <div>
</section>

Copy link
Author

Choose a reason for hiding this comment

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

It's done ;)

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.

4 participants