Skip to content

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