html-form by DominikaKarmel · Pull Request #798 · mate-academy/layout_html-form

DominikaKarmel · 2023-06-01T09:55:56Z

Radoslaw-Czerniawski

Left a few comments also:

again take a look at the task requirements and implement accordingly
there are a lot of indent and style errors adjust according to comments / checklist
don't use div's for everything try to use more semantic html (appropriate tags)

Radoslaw-Czerniawski · 2023-06-01T14:43:23Z

src/index.html

  <body>
-    <h1>HTML Form</h1>
    <script type="text/javascript" src="./main.js"></script>
+    <form action="http://localhost:8080/api" method="post"


action according to requirements should be https://mate-academy-form-lesson.herokuapp.com/create-application

Also formatting is wrong, adjust according to the checklist p. 4

Radoslaw-Czerniawski · 2023-06-01T14:50:16Z

src/index.html

    <script type="text/javascript" src="./main.js"></script>
+    <form action="http://localhost:8080/api" method="post"
+    class="form">
+      <fieldset class="form__group">


Generally you don't have any empty lines between elements making this code very hard to read, for example you should always have empty line between siblings:

<div> <span>first child</span> <p>second child</p> </div>

there is an exception to this rule, for example when you have a bunch of similar elements like in ul

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

Radoslaw-Czerniawski · 2023-06-01T14:51:03Z

src/index.html

+          >
+        </div>
+        <div class="form-field">
+          Name:


Such text should be a label for appropriate input so upon clicking on it correct field is focused

Radoslaw-Czerniawski

Left a few comments, don't adjust only the places comments are placed but based on them correctly format rest of your code, you have a lot of unnecessary divs and some wrong formatting

Radoslaw-Czerniawski · 2023-06-05T12:13:39Z

src/index.html

+        </div>
+      </fieldset>
+
+      <div class="form-submit">


There is no need for this button to be wrapped in div

Radoslaw-Czerniawski · 2023-06-05T12:14:36Z

src/index.html

+              type="password"
+              name="password"
+              autocomplete="off"
+              required minlength="5"


All attributes here should be on separate lines

Radoslaw-Czerniawski · 2023-06-05T12:17:40Z

src/index.html

+      <fieldset class="form__group">
+        <legend>Personal information</legend>
+
+        <div class="form__field">


I assume you are using this div to wrap all elements in a block elements so it takes the whole available horizontal space, there is no need for that if you already wrap your label and input in label element. Remove this div

Also there should be an empty line between surname and input

It should look like this:

<label class="form__field"> Surname: <input type="text" name="surname" autocomplete="off" > </label> <label class="form__field"> Name: <input type="text" name="name" autocomplete="off" >

Adjust rest of you elements and remove unnecessary divs

Also consider name a required attribute for the input, after form is submitted values from inputs that don't have names are omitted from the payload, structure of the payload is such that the input's name forms a property with corresponding value from the input on the submit payload

You may see this result in network tab after submitting your form, input's that didn't have names are not submitted

It should look like this:

<label class="form__field"> Surname: <input type="text" name="surname" autocomplete="off" > </label> <label class="form__field"> Name: <input type="text" name="name" autocomplete="off" >

Adjust rest of you elements and remove unnecessary divs

Hello Radoslaw, if i remove this divs then i'll have elements inline. How handle it?

Just add to your .form__field css class value display: block it should work. Then you can remove the div elements.

Radoslaw-Czerniawski · 2023-06-05T12:34:56Z

src/index.html

+
+          <select id="brandPicker" name="cars"
+          required multiple>
+            <option value="bmw">BMW</option>


These options shouln't have empty lines between them as they are repetitions of the same tag

Radoslaw-Czerniawski · 2023-06-05T12:35:52Z

src/index.html

+        <div class="form__field">
+          <label for="brandPicker">What are your favorite brand of cars?</label>
+
+          <select id="brandPicker" name="cars"


Formatting here is wrong, see how it's done for example on you inputs

Radoslaw-Czerniawski · 2023-06-05T12:36:19Z

src/index.html

+        </div>
+
+        <div>
+          How do you rate our work?


This should be a label for the below input

Radoslaw-Czerniawski · 2023-06-05T12:37:01Z

src/index.html

+
+            <option value="lada">Lada</option>
+
+            <option value=" " disabled> </option>


There is no need for this empty option, remove it

Radoslaw-Czerniawski · 2023-06-05T12:43:47Z

src/index.html

+      </fieldset>
+
+      <fieldset class="form__group">
+        <legend>An interesting fact about you!</legend>


As stated in the previous review all siblings should have an empty line between them apart from some specials situations with repeated tags:

<fieldset class="form__group"> <legend>An interesting fact about you!</legend> <div class="form__field"> <span>Do you love cats?</span> <label for="love">Yes</label> <input type="radio" id="love" name="cats" value="yes" > <label for="hate">No</label> <input type="radio" id="hate" name="cats" value="no" > </div>

Also see that I have wrapped you Do you love cats? text in a span,div element is a container to group up your elements for the purpose their styling/positioning, text alone is not an element and should be nested inside a tag (label wrapping a text and input is one of the exceptions from that rule)

Radoslaw-Czerniawski · 2023-06-05T12:49:51Z

src/index.html

+              disabled>
+            </option>
+
+            <option value="yes"


Wrong formatting, adjust similarly to how attributes are spaced on you inputs, also you don't need empty lines between options as these are same tags

DominikaKarmel · 2023-06-07T09:59:41Z

Hello Radoslaw, I don't know what You mean about unnecessary divs, I have problem with that, because if I leave only labels everything is inline... You mean I should change something in CSS? I did the rest of the comments.

darokrk

Some small improvements need to be fixed and we can move forward 👍🏽

darokrk · 2023-06-09T06:40:32Z

src/index.html

+            Surname:
+
+            <input
+              type="text"


Add a 'name' attribute to the input field for 'Surname'. Example: name="surname"

darokrk · 2023-06-09T06:40:32Z

src/index.html

+            How old are You?
+
+            <input
+              type="number"


Change the 'name' attribute value to a more descriptive name for the age input field. Example: name="age"

darokrk · 2023-06-09T06:40:32Z

src/index.html

+            Full date of birth:
+
+            <input
+              type="date"


Add a 'name' attribute to the input field for 'Full date of birth'. Example: name="birthdate"

darokrk · 2023-06-09T06:40:32Z

src/index.html

+          <label>
+            Name:
+
+            <input


Add a 'name' attribute to the input field for 'Name'. Example: name="name"

darokrk · 2023-06-09T06:46:42Z

src/index.html

+      <fieldset class="form__group">
+        <legend>Personal information</legend>
+
+        <div class="form__field">


Just add to your .form__field css class value display: block it should work. Then you can remove the div elements.

DorotaLeniecDev

few comment from my side

DorotaLeniecDev · 2023-06-12T19:53:24Z

src/index.html

+        <div>
+          <label>
+            I accept the term of the agreement
+
+            <input
+              type="checkbox"
+              name="agreement"
+              required
+            >
+          </label>
+        </div>


Why you wrapped this label with div element instead of adding form_field class as for other labels? You should try to be consistant with the way you write your code :)

DorotaLeniecDev · 2023-06-12T19:54:00Z

src/index.html

+          >
+        </label>
+
+        <div>


Same question here

DorotaLeniecDev · 2023-06-12T19:56:07Z

src/index.html

+        <label class="form__field">
+          What are your favorite brand of cars?
+
+          <select id="brandPicker" name="cars"


You should fix formatting here as Radek mentioned

Suggested change

<select id="brandPicker" name="cars"

<select

id="brandPicker"

name="cars"

required

multiple

>

DorotaLeniecDev · 2023-06-12T19:57:23Z

src/index.html

+      </fieldset>
+
+      <button type="submit">Submit</button>
+


Remove empty line, it is not needed between parent and child elements

Radoslaw-Czerniawski

One last mistake left, after it's corrected we'r good :D

Radoslaw-Czerniawski · 2023-06-13T17:49:29Z

src/index.html

+        </label>
+
+        <div>
+          <span>Would you recommend us?</span>


Change this to label for the select and get rid of the div

DorotaLeniecDev

Good job :)

html-form

22d3147

Radoslaw-Czerniawski suggested changes Jun 1, 2023

View reviewed changes

layout_html-form

507605a

DominikaKarmel requested a review from Radoslaw-Czerniawski June 5, 2023 10:36

Radoslaw-Czerniawski suggested changes Jun 5, 2023

View reviewed changes

layout_html-form

74e2d31

DominikaKarmel requested a review from Radoslaw-Czerniawski June 7, 2023 09:57

darokrk suggested changes Jun 9, 2023

View reviewed changes

layout_html-form

72c24ef

DominikaKarmel requested a review from darokrk June 12, 2023 07:09

DorotaLeniecDev suggested changes Jun 12, 2023

View reviewed changes

layout_html-form

8c89f09

DominikaKarmel requested a review from DorotaLeniecDev June 13, 2023 08:48

Radoslaw-Czerniawski suggested changes Jun 13, 2023

View reviewed changes

src/index.html

</label>

<div>

<span>Would you recommend us?</span>

Copy link

Radoslaw-Czerniawski Jun 13, 2023

Choose a reason for hiding this comment

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

Change this to label for the select and get rid of the div

layout_html-form

32231db

DominikaKarmel requested a review from Radoslaw-Czerniawski June 14, 2023 07:51

DorotaLeniecDev approved these changes Jun 14, 2023

View reviewed changes


		<option value="lada">Lada</option>

		<option value=" " disabled> </option>

+                        <label>
+                          Name:
+                          <input

Conversation

DominikaKarmel commented Jun 1, 2023

Uh oh!

Radoslaw-Czerniawski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Radoslaw-Czerniawski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DominikaKarmel commented Jun 7, 2023

Uh oh!

darokrk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DorotaLeniecDev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Radoslaw-Czerniawski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DorotaLeniecDev left a comment

Choose a reason for hiding this comment