Skip to content

add task solution#784

Open
spojrzenie wants to merge 4 commits into
mate-academy:masterfrom
spojrzenie:develop
Open

add task solution#784
spojrzenie wants to merge 4 commits into
mate-academy:masterfrom
spojrzenie:develop

Conversation

@spojrzenie

Copy link
Copy Markdown

No description provided.

@spojrzenie

Copy link
Copy Markdown
Author

@darokrk darokrk left a comment

Copy link
Copy Markdown

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 👍🏽

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

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

@darokrk darokrk Apr 27, 2023

Copy link
Copy Markdown

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

Comment thread src/index.html
type="password"
id="password"
name="password"
>

Copy link
Copy Markdown

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.

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

Copy link
Copy Markdown

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.

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.

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

Comment thread src/index.html Outdated
name="comments"
id="comments"
cols="30"
rows="10"

Copy link
Copy Markdown

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

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

Copy link
Copy Markdown

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

Comment thread src/index.html

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

Copy link
Copy Markdown

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

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

<div class="form-field">
<label for="surname">Surname</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.

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

Comment thread src/index.html
</div>
</fieldset>

<div class="form-field">

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
Author

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

@spojrzenie

Copy link
Copy Markdown
Author

Should i click button "Close with comment"?

@darokrk darokrk left a comment

Copy link
Copy Markdown

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

darokrk commented Apr 28, 2023

Copy link
Copy Markdown

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