Skip to content

add task solution#794

Open
Luk2asz wants to merge 3 commits intomate-academy:masterfrom
Luk2asz:develop
Open

add task solution#794
Luk2asz wants to merge 3 commits intomate-academy:masterfrom
Luk2asz:develop

Conversation

@Luk2asz
Copy link
Copy Markdown

@Luk2asz Luk2asz commented May 20, 2023

Copy link
Copy Markdown

@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 formating and use class for styling

Comment thread src/index.html Outdated
Comment on lines +20 to +22
<input type="text"
name="surname" autocomplete="off"
>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fix formating in this file. Check the checklist for guidelince. It should be formatet like this:

Suggested change
<input type="text"
name="surname" autocomplete="off"
>
<input
type="text"
name="surname"
autocomplete="off"
>

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I have corrected my code, please check now

Comment thread src/index.html Outdated
<fieldset>
<legend>Personal information</legend>
<div class="form">
<label>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Dont wrapp inputs with label.

Suggested change
<label>
<label for="surname">Surname:</label>

Add appropriate ids to form elements do the labels is connected

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I have corrected my code, please check now

Comment thread src/index.html Outdated
<form action="https://mate-academy-form-lesson.herokuapp.com/create-application" method="post">
<fieldset>
<legend>Personal information</legend>
<div class="form">
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Change class name so it is not missleading. This HTML does not represents form, it is more like field-wrapper

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I have corrected my code, please check now

Comment thread src/index.html Outdated
Comment thread src/style.css Outdated
margin-bottom: 10px;
}

fieldset {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Dont style using tags, add propper class name and style using it

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I have corrected my code, please check now

@Luk2asz Luk2asz requested a review from DorotaLeniecDev May 23, 2023 17:55
Copy link
Copy Markdown

@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 👍🏽 give approval, but I have added two suggestions for improvement which take a look at :)

Comment thread src/index.html
id="no"
>

<label for="no">Yes</label>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The label text for the 'no' radio button should be 'No' instead of 'Yes'

Comment thread src/style.css
margin-bottom: 10px;
}

.chapter-wrapper {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would suggest here to give a more descriptive name for this class, maybe something like .fieldset-wrapper or .form-section :)

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.

3 participants