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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Develop #5346

wants to merge 1 commit into from

Conversation

RareHelix
Copy link

Header height is set in 1 place (for the links)
Content is vertically centered (for any header height). - [x]
CSS is used to show all letters in Uppercase (don't type them in HTML) - [x]
Logo is an image wrapped with a link - [x]
CSS Variable is used for a blue color
Pseudo-element is used for a blue line below the active link - [x]
Code follows all the [Code Style Rules ❗️] - x

Copy link

@anastasiiavorobiova anastasiiavorobiova left a comment

Choose a reason for hiding this comment

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

Great work! Almost done!

src/style.css Outdated
Comment on lines 79 to 85
a[data-qa]:hover {
color: #00acdc;
}

/* [data-qa='hover'] {
color: #00acdc;
} */

Choose a reason for hiding this comment

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

Suggested change
a[data-qa]:hover {
color: #00acdc;
}
/* [data-qa='hover'] {
color: #00acdc;
} */

src/style.css Outdated
}

.is-active::after {
color: #00acdc;

Choose a reason for hiding this comment

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

Consider using variables for repeating colors

Copy link
Author

@RareHelix RareHelix left a comment

Choose a reason for hiding this comment

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

Змінила ноуд на 20, можливо через це не пропускає

Copy link

@volodymyr-soltys97 volodymyr-soltys97 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 👍
Let's make your code better

src/index.html Outdated
Comment on lines 42 to 23
</a>
<nav class="nav">

Choose a reason for hiding this comment

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

Suggested change
</a>
<nav class="nav">
</a>
<nav class="nav">

src/index.html Outdated
Comment on lines 52 to 31
</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.

Add the blank line between the links

Suggested change
</li>
<li class="nav__item">
</li>
<li class="nav__item">

src/style.css Outdated
Comment on lines 5 to 7
html {
margin: 0;
}

Choose a reason for hiding this comment

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

Suggested change
html {
margin: 0;
}

Copy link
Author

Choose a reason for hiding this comment

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

Не розумію,що тут виправити?

Choose a reason for hiding this comment

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

видалити цей селектор

src/style.css Outdated
Comment on lines 83 to 85
a[data-qa]:hover {
color: var(--main-color);
}

Choose a reason for hiding this comment

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

Suggested change
a[data-qa]:hover {
color: var(--main-color);
}

Copy link
Author

Choose a reason for hiding this comment

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

Не розумію,що тут виправити?

Choose a reason for hiding this comment

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

видалити

src/style.css Outdated
Comment on lines 75 to 70
a.is-active {
color: var(--main-color);
}

Choose a reason for hiding this comment

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

Suggested change
a.is-active {
color: var(--main-color);
}

Copy link
Author

Choose a reason for hiding this comment

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

Не розумію,що тут виправити?

Choose a reason for hiding this comment

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

видалити теж

src/style.css Outdated
Comment on lines 5 to 7
html {
margin: 0;
}

Choose a reason for hiding this comment

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

видалити цей селектор

src/style.css Outdated
Comment on lines 75 to 70
a.is-active {
color: var(--main-color);
}

Choose a reason for hiding this comment

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

видалити теж

src/style.css Outdated
Comment on lines 83 to 85
a[data-qa]:hover {
color: var(--main-color);
}

Choose a reason for hiding this comment

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

видалити

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.

you still haven't fix all prev comments
image

Copy link

@volodymyr-soltys97 volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

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

Hi, you need to fix all comments from the previous review, if you need help feel free in the fe_chat

@RareHelix
Copy link
Author

RareHelix commented Sep 10, 2024 via email

Copy link

@anastasiiavorobiova anastasiiavorobiova left a comment

Choose a reason for hiding this comment

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

Great work!

text-transform: uppercase;
}

.nav_list:first-child {

Choose a reason for hiding this comment

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

Suggested change
.nav_list:first-child {
.nav__item:first-child {

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