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.

Sign up for GitHub

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

Jump to bottom

Moyo-Header(add task solution) #4911

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

Moyo-Header(add task solution) #4911

wants to merge 3 commits into from

Conversation

gorbachovam00
Copy link

volodymyr-soltys97
volodymyr-soltys97 suggested changes Apr 30, 2024
View reviewed changes
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
You need to show a blue color for text for all these links when hoverign
image

src/index.html Outdated
Comment on lines 33 to 34
<div class="container">
<div class="header__container">
Copy link

Choose a reason for hiding this comment

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

These divs are redundant, so you need to remove it

Suggested change
<div class="container">
<div class="header__container">

src/index.html Outdated
Comment on lines 41 to 43
<nav class="header__nav">
<ul class="header__menu">
<li class="menu-item">
Copy link

Choose a reason for hiding this comment

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

Fix class everywhere

Suggested change
<nav class="header__nav">
<ul class="header__menu">
<li class="menu-item">
<nav class="nav">
<ul class="nav__list">
<li class="nav__item">

src/index.html Outdated
<li class="menu-item">
<a
href="#"
class="link link-1 is-active"
Copy link

Choose a reason for hiding this comment

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

Fix class everywhere

Suggested change
class="link link-1 is-active"
class="nav__link is-active"

src/index.html Outdated
Comment on lines 50 to 51
</li>
<li class="menu-item">
Copy link

Choose a reason for hiding this comment

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

Add a blank line between the links

Suggested change
</li>
<li class="menu-item">
</li>
<li class="menu-item">

src/index.html Outdated
Comment on lines 116 to 131
<!-- <!doctype html>
<html lang="en">
<head>
<meta charset="UTF-8" />
<meta
name="viewport"
content="width=device-width, user-scalable=no, initial-scale=1.0, maximum-scale=1.0, minimum-scale=1.0"
/>
<meta
http-equiv="X-UA-Compatible"
content="ie=edge"
/>
<title>Document</title>
</head>
<body></body>
</html> -->
Copy link

Choose a reason for hiding this comment

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

Remove comments

src/style.css
Comment on lines +14 to +17
font-family: Roboto, sans-serif;
font-size: 12px;
font-weight: 500;
line-height: 1.2;
Copy link

Choose a reason for hiding this comment

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

Move these styles to the html selector

src/style.css Outdated
Comment on lines 77 to 86
/* .container {
margin: 0 auto;
} */

/* .header__container {
display: flex;
justify-content: space-between;
align-items: center;
padding: 10px 50px;
}
Copy link

Choose a reason for hiding this comment

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

Remove comments everywhere

src/style.css Outdated
display: block;
position: absolute;
content: '';
width: 37px;
Copy link

Choose a reason for hiding this comment

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

Suggested change
width: 37px;
width: 100%;

src/style.css Outdated
height: 4px;
background-color: var(--color-header);
border-radius: 8px;
top: 56px;
Copy link

Choose a reason for hiding this comment

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

Suggested change
top: 56px;
bottom: 0;

anastasiiavorobiova
anastasiiavorobiova approved these changes Apr 30, 2024
View reviewed changes
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!

src/style.css Outdated
margin: 0;
}

ul li:not(:last-child) {
Copy link

Choose a reason for hiding this comment

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

Suggested change
ul li:not(:last-child) {
.nav__item:not(:last-child) {

src/style.css
text-decoration: none;
color: var(--color-text);
text-transform: uppercase;
overflow-x: hidden;
Copy link

Choose a reason for hiding this comment

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

Suggested change
overflow-x: hidden;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anastasiiavorobiova anastasiiavorobiova anastasiiavorobiova approved these changes

@volodymyr-soltys97 volodymyr-soltys97 Awaiting requested review from volodymyr-soltys97

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gorbachovam00 @volodymyr-soltys97 @anastasiiavorobiova