Skip to content

Conversation

@spojrzenie
Copy link

No description provided.

@spojrzenie
Copy link
Author

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.

Good job, some small shortcomings to improve and it should be ok 👍🏽

src/style.css Outdated
margin-bottom: 20px;
}

/* Usuń margines dolny dla ostatniego .form-field wewnątrz fieldset */
Copy link

@darokrk darokrk Apr 27, 2023

Choose a reason for hiding this comment

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

Please change the comment to English, for better understanding and consistency or remove it

type="password"
id="password"
name="password"
>
Copy link

Choose a reason for hiding this comment

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

Add minlength and maxlength attributes for the password input to define password strength requirements.

src/index.html Outdated
<input
type="color"
id="color"
name="fav-color"
Copy link

Choose a reason for hiding this comment

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

if we follow camelCase convention like you named loveCats this should be name="favColor" also for bedtime & favcars etc.

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

fieldset {
Copy link

Choose a reason for hiding this comment

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

don't style elements by tag, rather use some class name here like .form-fieldset

src/index.html Outdated
name="comments"
id="comments"
cols="30"
rows="10"
Copy link

Choose a reason for hiding this comment

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

Think you rows value here is to big, it should be rather 3

src/index.html Outdated
id="cars"
multiple
>
<option value="audi">Audi</option>
Copy link

Choose a reason for hiding this comment

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

try options input here like:
BMW
Audi
Lada


<div class="form-field">
<label for="bedTime">What time do you go to bed?</label>
<input
Copy link

Choose a reason for hiding this comment

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

add step="2" attribute to have seconds values

src/index.html Outdated
<legend>Personal information</legend>

<div class="form-field">
<label for="surname">Surname</label>
Copy link

Choose a reason for hiding this comment

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

You missed colon for each label in your file, please add : e.g. Surname:

</div>
</fieldset>

<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.

do we need this button to be grouped in this div?

@spojrzenie
Copy link
Author

Good job, some small shortcomings to improve and it should be ok 👍🏽

Thank you, i'll fix it later.
Did i execute Pull requests correctly?
Because it's probably not done with a "fork"? and with my username.
Did I do it right though? :)

@spojrzenie
Copy link
Author

Ok, i think i fixed everything. Can you accept my changes so I can move on?

@spojrzenie
Copy link
Author

Should i click button "Close with comment"?

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.

Task approved! Nicely done, good job 🥇

@darokrk
Copy link

darokrk commented Apr 28, 2023

Good job, some small shortcomings to improve and it should be ok 👍🏽

Thank you, i'll fix it later. Did i execute Pull requests correctly? Because it's probably not done with a "fork"? and with my username. Did I do it right though? :)

Yes you did it correctly :)

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.

2 participants