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

add task solution #801

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

add task solution #801

wants to merge 6 commits into from

Conversation

aRabiej
Copy link

@aRabiej aRabiej commented Jun 15, 2023

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.

There is no link with demo

@aRabiej
Copy link
Author

aRabiej commented Jun 18, 2023

should be visible now

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.

Check this checklist https://github.com/mate-academy/layout_html-form/blob/master/checklist.md, you need to fix:

  1. Formatting, make sure the indentation is correct, also add empty lines between sibling elements like in this example:
        <div class="field_wrapper">
          <label for="surname">Surname: </label>

          <input
            type="text"
            name="surname"
            id="surname"
            autocomplete="off"
          >
        </div>
  1. Also avoid stylin by using element selector, add class to the element and style it using class like:
    html file:
        <div class="field_wrapper">
          <label for="surname">Surname: </label>

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

css file:

.field_wrapper {
  margin-bottom: 10px
}
  1. Add proper labels to all inputs so when the user clicks label it will focus on the proper input (for-id relation):
          <label for="name">Name: </label>

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

@aRabiej aRabiej requested a review from DorotaLeniecDev June 28, 2023 18:48
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.

The code needs some improvements :)

Please look at Radek's mention before the link with the demo is still missing.

src/index.html Outdated
type="date"
name="date"
id="date"
value=" "
Copy link

Choose a reason for hiding this comment

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

Empty space as a default value in input is not a good practice. Please remove this attribute value=" " it's redundant here

src/index.html Outdated
type="text"
name="name"
id="name"
value=" "
Copy link

Choose a reason for hiding this comment

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

Empty space as a default value in input is not a good practice. Please remove this attribute value=" " it's redundant here

src/index.html Outdated
type="text"
name="surname"
id="surname"
value=" "
Copy link

Choose a reason for hiding this comment

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

Empty space as a default value in input is not a good practice. Please remove this attribute value=" " it's redundant here

src/index.html Outdated
type="email"
name="email"
id="email"
value=" "
Copy link

Choose a reason for hiding this comment

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

Empty space as a default value in input is not a good practice. Please remove this attribute value=" " it's redundant here

src/index.html Outdated
<input
type="color"
name="color"
value=" "
Copy link

Choose a reason for hiding this comment

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

Empty space as a default value in input is not a good practice. Please remove this attribute value=" " it's redundant here

src/index.html Outdated
name="time"
id="time"
step="2"
value=" "
Copy link

Choose a reason for hiding this comment

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

Empty space as a default value in input is not a good practice. Please remove this attribute value=" " it's redundant here

src/style.css Outdated
}

input {
.field_wrapper {
Copy link

Choose a reason for hiding this comment

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

following the BEM naming convention this name should have double underscore mark field__wrapper

src/style.css Outdated

label {
margin-top: 10px;
.box_style {
Copy link

Choose a reason for hiding this comment

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

following the BEM naming convention this name should have double underscore mark box__style

@aRabiej aRabiej requested a review from darokrk June 29, 2023 16:33
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 correction needed, check my comments

src/index.html Outdated
name="email"
id="email"
placeholder="[email protected]"
multiple

Choose a reason for hiding this comment

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

You can use multiple attribute with input type email, but I think that in this case more suitable would be to add multiple attribute to select tag, as this is registration form so we are allowing one email value :)

src/index.html Outdated
<label for="user-password">Password: </label>

<input
name="user-password"

Choose a reason for hiding this comment

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

Value of name attribute should be in camelCase like userPassword

src/index.html Outdated
name="user-password"
id="user-password"
type="password"
required 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
required autocomplete="off"
required
autocomplete="off"

src/index.html Outdated
Comment on lines 129 to 132
<input
type="color"
name="color"
>

Choose a reason for hiding this comment

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

Label for this field is not clickable(when clicking label it should focuse on the input).
Add id which coresponds with for attribute in label

Suggested change
<input
type="color"
name="color"
>
<input
type="color"
name="color"
id="color"
>

src/index.html Outdated
Comment on lines 149 to 155
<textarea
name="cars"
id="cars"
rows="4"
cols="2"
>BMW Audi Lada
</textarea>

Choose a reason for hiding this comment

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

Here more suitable would be to use select tag instead of textarea
Here is an example:

<select name="cars" id="cars" multiple>
  <option value="volvo">Volvo</option>
  <option value="saab">Saab</option>
  <option value="opel">Opel</option>
  <option value="audi">Audi</option>
</select>

This will also fix your test results :)

<div class="field__wrapper">
<label for="comment">Comments: </label>

<textarea

Choose a reason for hiding this comment

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

Add id

src/index.html Outdated
Comment on lines 174 to 178
name="comment"
rows="2"
cols="20"
>
</textarea>

Choose a reason for hiding this comment

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

<textarea> is a tricky tag to format:
It requires you to put a closing tag on the same line as the > of the opening tag, not to have default content, and be able to see the placeholder.
At the same time, if it has several attributes, you need to put each of them on its own line.
So, use the following formatting where > is moved 1 position left from the normal alignment:

  <textarea
    class="..."
    placeholder="..."
 ></textarea>

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

Choose a reason for hiding this comment

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

Add empty line between sibling elements

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

Choose a reason for hiding this comment

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

Use more descriptive name and id value like recommendation

src/index.html Outdated
Comment on lines 186 to 187
</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.

Add empty line between sibling elements

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

@aRabiej aRabiej requested a review from DorotaLeniecDev June 30, 2023 13:55
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.

Some minor errors, correct them and we'r good :D

src/index.html Outdated
<h1>HTML Form</h1>
<form
action="https://mate-academy-form-lesson.herokuapp.com/create-application"
method="get"

Choose a reason for hiding this comment

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

Method here should be POST rather than GET -> https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods

src/index.html Outdated
type="radio"
id="yes"
name="cat"
value="Y"

Choose a reason for hiding this comment

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

This value is very vague, it would be better here to reflect what is checked at the input -> Yes and below No

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.

4 participants