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 #784

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

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