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

Added task solution #782

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

Conversation

kyrylomanko
Copy link

@kyrylomanko kyrylomanko commented Apr 26, 2023

Copy link

@Anastasiia-Svintsova Anastasiia-Svintsova left a comment

Choose a reason for hiding this comment

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

  • The user should be able to choose only one option (yes or no). Fix these radio buttons, pelase
Screen.Recording.2023-04-27.at.12.36.45.mov

Comment on lines +3 to +4
- [DEMO LINK](https://kyrylomanko.github.io/layout_html-form/)
- [TEST REPORT LINK](https://kyrylomanko.github.io/layout_html-form/report/html_report/)

Choose a reason for hiding this comment

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

You should add these links to the PR description

src/index.html Outdated
id="surname"
name="surname"
autocomplete="off"
><br>

Choose a reason for hiding this comment

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

Why do you need these br tags everywhere? Remove them

Suggested change
><br>
>

src/index.html Outdated
<label for="surname">Surname:</label>
<input
cletype="text"
class="distance"

Choose a reason for hiding this comment

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

Use more meaningful class names. For example:

Suggested change
class="distance"
class="form-field"

src/index.html Outdated
Comment on lines 59 to 61
>

</fieldset>

Choose a reason for hiding this comment

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

Suggested change
>
</fieldset>
>
</fieldset>

src/index.html Outdated
Comment on lines 79 to 81
>

</fieldset>

Choose a reason for hiding this comment

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

Don't add spaces between parent and child elements

Suggested change
>
</fieldset>
>
</fieldset>

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

fieldset {

Choose a reason for hiding this comment

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

Don't style tags, use class selector instead

Copy link

@witflash witflash left a comment

Choose a reason for hiding this comment

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

Almost done but just one thing:
image
Could you check if you use these attributes at least one time?

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.

4 participants