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 test solution #800

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

Conversation

@mkyloa mkyloa closed this Jun 6, 2023
@mkyloa mkyloa reopened this Jun 6, 2023
@mkyloa mkyloa requested a review from ihor-jpeg June 6, 2023 10:51
@mkyloa mkyloa closed this Jun 6, 2023
@mkyloa mkyloa reopened this Jun 6, 2023
Copy link

@mkyloa mkyloa left a comment

Choose a reason for hiding this comment

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

test

Copy link

@mkyloa mkyloa left a comment

Choose a reason for hiding this comment

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

test

@mkyloa mkyloa self-requested a review June 6, 2023 11:00
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 HTML formating and remove unnecessary class names

src/index.html Outdated
Comment on lines 6 to 7
<meta name="viewport"
content="width=device-width, user-scalable=no, initial-scale=1.0, maximum-scale=1.0, minimum-scale=1.0"

Choose a reason for hiding this comment

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

Fix formatting

Suggested change
<meta name="viewport"
content="width=device-width, user-scalable=no, initial-scale=1.0, maximum-scale=1.0, minimum-scale=1.0"
<meta
name="viewport"
content="width=device-width, user-scalable=no, initial-scale=1.0, maximum-scale=1.0, minimum-scale=1.0"
>

src/index.html Outdated
<body>
<h1>HTML Form</h1>

Choose a reason for hiding this comment

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

Spaces between parent and child are not needed, remove

src/index.html Outdated
Comment on lines 16 to 17
<form action="https://mate-academy-form-lesson.herokuapp.com/create-application"
method="post" name="personal-information">

Choose a reason for hiding this comment

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

Fiz formatting:
Also name should be camelCase

Suggested change
<form action="https://mate-academy-form-lesson.herokuapp.com/create-application"
method="post" name="personal-information">
<form
action="https://mate-academy-form-lesson.herokuapp.com/create-application"
method="post"
name="personal-information"
>

src/index.html Outdated
Comment on lines 22 to 29
<label for="surname">Surname:
<input
type="text"
autocomplete="off"
id="surname"
name="surname"
>
</label>

Choose a reason for hiding this comment

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

Fix formatting:

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

src/index.html Outdated
</div>

<div class="input name">
<label for="name">Name:

Choose a reason for hiding this comment

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

Same here, check all documnent and fix formatting

src/index.html Outdated
>
</label>
</div>

Choose a reason for hiding this comment

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

remove line

src/index.html Outdated
<input
type="radio"
name="cats"
id="cats yes"

Choose a reason for hiding this comment

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

Do not add two ids, you can use for example catLoverYes


<div class="input cars">
<label for="cars">What are your favorite brand of cars?
<select

Choose a reason for hiding this comment

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

Fix formatting

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

<fieldset class="form-field personal-information">

Choose a reason for hiding this comment

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

You dont need additional classes for fieldset, remove in whole HTML and use only form-field class, I would suggest to rename it to form-fieldset for consistancy

src/index.html Outdated

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

Choose a reason for hiding this comment

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

Same here, use single class for styling input container and name class more descriptively like form-field

@arturgorniak
Copy link
Author

I made changes as suggested, it should be fine now.

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.

Please fix code indentations in whole file and we are ready to go 👍🏽

<body>
<h1>HTML Form</h1>
<form
Copy link

Choose a reason for hiding this comment

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

Please fix the formatting of the code. You have to use the correct code indentations.

Look Dorota's mentions before and apply it in the whole code :)

    <form 
       action="https://mate-academy-form-lesson.herokuapp.com/create-application"
       method="post"
       name="personalInformation"
    >

@arturgorniak
Copy link
Author

done

@arturgorniak arturgorniak requested a review from darokrk June 9, 2023 14:40
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 and we are good to go

src/index.html Outdated
Comment on lines 5 to 7
<meta
charset="UTF-8"
>

Choose a reason for hiding this comment

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

Overall when there is less than 3 attributes in a tag there is no need for spreading it into several lines. We do it when the attribute list is long for readability so this should simply be:

Suggested change
<meta
charset="UTF-8"
>
<meta charset="UTF-8">

src/index.html Outdated
Comment on lines 10 to 11
content="width=device-width, user-scalable=no, initial-scale=1.0,
maximum-scale=1.0, minimum-scale=1.0"

Choose a reason for hiding this comment

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

Dont spread attribute value, put it in a one line

Suggested change
content="width=device-width, user-scalable=no, initial-scale=1.0,
maximum-scale=1.0, minimum-scale=1.0"
content="width=device-width, user-scalable=no, initial-scale=1.0, maximum-scale=1.0, minimum-scale=1.0"

src/index.html Outdated
Comment on lines 132 to 144
<input
type="radio"
name="cats"
id="catsYes"
>
Yes

<input
type="radio"
name="cats"
id="catsNo"
>
No

Choose a reason for hiding this comment

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

Make Yes and No values a clicable lables so it focuses on the correct input when clicked

<label for="cars">
What are your favorite brand of cars?

<select

Choose a reason for hiding this comment

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

Fix formating for this select tag, add indentations

src/index.html Outdated
Comment on lines 177 to 179
name="cars"
id="cars"
multiple>

Choose a reason for hiding this comment

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

Fix formating for this select tag, add indentations like:

Suggested change
name="cars"
id="cars"
multiple>
<select
name="cars"
id="cars"
multiple
>

src/index.html Outdated
type="submit"
value="Submit"
>

Choose a reason for hiding this comment

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

remove empty line

@arturgorniak
Copy link
Author

Finally i had some time to fix this. Can i also please to run tests for hello world task?
https://github.com/arturgorniak/layout_hello-world

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