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

Closed
wants to merge 4 commits into from
Closed

Develop #4535

wants to merge 4 commits into from

Conversation

MykolaDotsenko
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

@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 improve your code

src/index.html Outdated
Comment on lines 11 to 12
<link rel="preconnect" href="https://fonts.gstatic.com"
crossorigin = "anonymous">

Choose a reason for hiding this comment

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

Fix code style everywhere

Suggested change
<link rel="preconnect" href="https://fonts.gstatic.com"
crossorigin = "anonymous">
<link
rel="preconnect"
href="https://fonts.gstatic.com"
crossorigin="anonymous"
>

src/index.html Outdated
Comment on lines 18 to 19
<a class="header__logo" href="#"
aria-label="Home">

Choose a reason for hiding this comment

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

Suggested change
<a class="header__logo" href="#"
aria-label="Home">
<a
class="header__logo"
href="#"
aria-label="Home"
>

src/index.html Outdated
</a>
<nav class="header__nav">
<ul class="header__list">
<li class="nav__item"><a href="#" class="nav__link is-active">Apple</a></li>

Choose a reason for hiding this comment

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

Need to fix code style everywhere and add the blank line between the links

Suggested change
<li class="nav__item"><a href="#" class="nav__link is-active">Apple</a></li>
<li class="nav__item">
<a href="#" class="nav__link is-active">Apple</a>
</li>

src/index.html Outdated
Comment on lines 28 to 29
<a href="#" class="nav__link"
data-qa="hover">Laptops & computers</a>

Choose a reason for hiding this comment

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

Suggested change
<a href="#" class="nav__link"
data-qa="hover">Laptops & computers</a>
<a
href="#"
class="nav__link"
data-qa="hover"
>
Laptops & computers
</a>

src/style.css Outdated
display: flex;
justify-content: space-between;
align-items: center;
background-color: #fff;

Choose a reason for hiding this comment

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

Default color

Suggested change
background-color: #fff;

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
padding: 0 50px;
}

.header img {

Choose a reason for hiding this comment

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

Use the classes selector for image

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.

додай атрибут клас для цієї картинки і використовуй його замість img

src/style.css Outdated
color: var(--main-color);
}

.nav__link.is-active::after {

Choose a reason for hiding this comment

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

Suggested change
.nav__link.is-active::after {
.is-active::after {

Copy link

@maxim2310 maxim2310 left a comment

Choose a reason for hiding this comment

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

Great job overall! Just couple tiny things needs to be fixed before approval)
next time if you will have questions about review or something else plz ask it in your fe_chat

src/style.css Outdated
display: flex;
justify-content: space-between;
align-items: center;
background-color: #fff;

Choose a reason for hiding this comment

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

мається на увазі що це і так дефолтний колір, то це правило тут не потрібне, просто видали його

src/style.css Outdated
padding: 0 50px;
}

.header img {

Choose a reason for hiding this comment

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

додай атрибут клас для цієї картинки і використовуй його замість img

Copy link

@maxim2310 maxim2310 left a comment

Choose a reason for hiding this comment

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

Need to fix last point

src/style.css Outdated
padding: 0 50px;
}

.header__logo img {

Choose a reason for hiding this comment

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

still not fixed

Copy link
Author

Choose a reason for hiding this comment

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

напиши як

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.

image

Copy link

@maxim2310 maxim2310 left a comment

Choose a reason for hiding this comment

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

if you have additional questions ask in fe_chat

src/style.css Outdated
padding: 0 50px;
}

.header__logo img {

Choose a reason for hiding this comment

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

image

src/index.html Outdated
href="#"
aria-label="Home"
>
<img src="./images/logo.png" alt="logo">

Choose a reason for hiding this comment

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

Suggested change
<img src="./images/logo.png" alt="logo">
<img class="header__img" src="./images/logo.png" alt="logo">

src/style.css Outdated
padding: 0 50px;
}

.header__logo img {

Choose a reason for hiding this comment

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

Suggested change
.header__logo img {
.header__img {

Copy link
Author

Choose a reason for hiding this comment

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

а в чому помилка коли на тег а стояв клас?

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.

Завжди краще робити саме таким чином

Комбіновані селектори уже мають вищу специфічність, це надійніше і зручніше потім підтримувати якщо селектор це просто 1 класс :)

Copy link

@maxim2310 maxim2310 left a comment

Choose a reason for hiding this comment

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

.

@MykolaDotsenko MykolaDotsenko closed this by deleting the head repository Feb 19, 2024
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