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

Develop #4895

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

Develop #4895

wants to merge 6 commits into from

Conversation

lanacodess
Copy link

  • DEMO LINK

  • TEST REPORT LINK

  • Header height is set in 1 place (for the links)

  • Content is vertically centered (for any header height)

  • CSS is used to show all letters in Uppercase (don't type them in HTML)

  • Logo is an image wrapped with a link

  • CSS Variable is used for a blue color

  • Pseudo-element is used for a blue line below the active link

  • Code follows all the Code Style Rules ❗️

Copy link

@nazarmatsevych nazarmatsevych left a comment

Choose a reason for hiding this comment

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

Very good start!
Please fix the comments below and try to pass failted tests.
If you have any questions - feel free to ask them in fe_chat

src/index.html Outdated
</li>
<li class="nav-item">
<a
href="#"

Choose a reason for hiding this comment

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

Add appropriate href value

Suggested change
href="#"
href="#Samsung"

src/index.html Outdated
Comment on lines 67 to 75
<li class="nav-item">
<a
href="#"
class="nav-link"
>
Smartphones
</a>
</li>
<li class="nav-item">

Choose a reason for hiding this comment

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

Suggested change
<li class="nav-item">
<a
href="#"
class="nav-link"
>
Smartphones
</a>
</li>
<li class="nav-item">
<li class="nav-item">
<a
href="#"
class="nav-link"
>
Smartphones
</a>
</li>
<li class="nav-item">

Add an empty line between multiple sibling blocks of html

src/style.css Outdated
body {
margin: 0;
padding: 0;
box-sizing: border-box;

Choose a reason for hiding this comment

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

Move-box-sizing to * selector

Choose a reason for hiding this comment

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

Still not fixed.

src/style.css Outdated
body {
margin: 0;
padding: 0;

Choose a reason for hiding this comment

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

Body doesn't have default padding

src/style.css Outdated
Comment on lines 11 to 13
ul {
list-style-type: none;
}

Choose a reason for hiding this comment

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

Reset default ul padding and margin

Suggested change
ul {
list-style-type: none;
}
ul {
list-style-type: none;
margin: 0;
padding: 0;
}

src/style.css Outdated

.nav-item {
position: relative;
padding: 22.5px 0;

Choose a reason for hiding this comment

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

Suggested change
padding: 22.5px 0;
padding: 23px 0;

It should help with failed tests

src/style.css Outdated
display: flex;
align-items: center;
justify-content: space-between;
margin: 0 50px;

Choose a reason for hiding this comment

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

Suggested change
margin: 0 50px;
padding: 0 50px;

Copy link

@vadiimvooo vadiimvooo left a comment

Choose a reason for hiding this comment

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

Still some comments were not fixed. Please feel free to ask for help in fe_chat to solve all the test in proper way.

src/style.css Outdated
body {
margin: 0;
padding: 0;
box-sizing: border-box;

Choose a reason for hiding this comment

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

Still not fixed.

src/style.css Outdated

.nav-item {
position: relative;
padding: 23px 0;

Choose a reason for hiding this comment

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

Add line-height for nav-link instead of this paddings to pass the tests

@lanacodess lanacodess requested a review from vadiimvooo April 29, 2024 18:24
Copy link

@nazarmatsevych nazarmatsevych 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, let's try to pass all failed tests

<!-- logo -->
<a
href="#"
class="logo"

Choose a reason for hiding this comment

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

Add font-size: 0 to logo class to remove this extra space
image

display: flex;
align-items: center;
justify-content: space-between;
padding: 0 50px;

Choose a reason for hiding this comment

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

add min-width: 1024px to limit minimal width of header due to design

Copy link

@etojeDenys etojeDenys 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

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