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

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

Conversation

KarolinaWiktoria
Copy link

Copy link

@Radoslaw-Czerniawski Radoslaw-Czerniawski left a comment

Choose a reason for hiding this comment

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

Looks very good, needs some tinkering to meet the requirements

src/index.html Outdated

<label class="input-row">
Surname:
<input name="surname" autocomplete="off">

Choose a reason for hiding this comment

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

Task requirements specify that you should use types attributes, even though inputs by default are type='text' mark it like so explicitly

Choose a reason for hiding this comment

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

Also you forgot about some of these attributes:
image

<legend>Personal information</legend>

<label class="input-row">
Surname:

Choose a reason for hiding this comment

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

This way of wrapping with label is kinda unorthodox, I'd suggest in the future do it the "regular way" without wrapping and using 'for' and 'id' attributes for the sake of readability,

Choose a reason for hiding this comment

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

For now just add an empty line between all label titles and inputs e.g.:

<label class="input-row">
  Surname:

  <input name="surname" autocomplete="off">
</label>

Do it for all your labels with inputs

Yes
</label>

<label>

Choose a reason for hiding this comment

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

Fix the indent:

<label>
  <input
    type="radio"
    name="cats"
    value="no"
  > 

  No
</label>

Copy link

@Radoslaw-Czerniawski Radoslaw-Czerniawski left a comment

Choose a reason for hiding this comment

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

Last change and we'r good :)

src/index.html Outdated
<input
type="range"
name="rate"
minlength="0"

Choose a reason for hiding this comment

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

Input type range should have min and max attributes rather than minlength and maxlength, these are more appropriate for inputs with text, for example you want you password to be of some minimal length.

Copy link
Author

Choose a reason for hiding this comment

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

Okay :). Thank you. I hope now everything is correct :-)

Copy link

@Radoslaw-Czerniawski Radoslaw-Czerniawski left a comment

Choose a reason for hiding this comment

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

I see on the demo that you've adjusted it correctly but I don't see the changes in this pr for some reason, I think you commited these changes to wrong branch or something.

@KarolinaWiktoria
Copy link
Author

hmmm... no Idea ;). Hopefully now you can see it in PR..

Thank you and have a good weekend! :)

Copy link

@Radoslaw-Czerniawski Radoslaw-Czerniawski 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, looks perfect :)

@KarolinaWiktoria
Copy link
Author

🙏🏼finally 💪🏼😀

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