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

add task solution #5326

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

add task solution #5326

wants to merge 4 commits into from

Conversation

yurzxw
Copy link

@yurzxw yurzxw commented Aug 31, 2024 •
edited
Loading

  • 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 ❗️

vadiimvooo
vadiimvooo suggested changes Aug 31, 2024
View reviewed changes
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.

Good job, but you have to fix your code according to comments below. Feel free to ask for help in fe_chat.

src/style.css Outdated
Comment on lines 51 to 52
bottom: -20px;
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
bottom: -20px;
width: 37px; /* Довжина лінії */
bottom: 0;
width: 100%;

src/index.html Outdated
</head>
<body>
<h1>Moyo header</h1>
<header class="header">
<container class="container">
Copy link

Choose a reason for hiding this comment

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

Suggested change
<container class="container">

Copy link

Choose a reason for hiding this comment

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

Looks redundant. Please remove it.

src/index.html Outdated
<container class="container">
<a href="#">
<img
src="https://s3-alpha-sig.figma.com/img/833f/9aab/47d660efd384318772b6b8040db831aa?Expires=1725840000&Key-Pair-Id=APKAQ4GOSFWCVNEHN3O4&Signature=MJRW8Yu0t9rfw~RbBcB~YImlgu22Gqhw2VLCvrJt9QpHubYRZwarK8rWfd0cXYEq3UdLnf2pTYuHtGkIMDr2rGTTba0KbaiAY4iTHQLsAoyHlo~eXIBi-CbDvG1aArEN2ybS2cDYwwOLuHm78zORnVD8AIhelkjjsme9N7SqEsADP1J0Kh0aKujhSICCib5MWOXbMHON4vv8ZK2qmlpdH8dJdm1h-Ahcsvw3pRFygjDfC9e0kZmk1ogcKrzrPTnxQbUeclnN-budN5yWzdz2icxTPVf7qdE98Sx9nKSkHLTXcbRKQZlEdqYer8R2A6sL4FoBsP1KL2IVJuFVP9v83w__"
Copy link

Choose a reason for hiding this comment

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

Just upload image inside your project and import it.

src/style.css Outdated
@@ -1,3 +1,62 @@
body {
margin: 0;
font-size: 12px;
font-family: Roboto, sans-serif;
Copy link

Choose a reason for hiding this comment

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

use html tag to import font-family

Copy link

Choose a reason for hiding this comment

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

Not fixed
image

src/style.css Outdated
color: #00acdc;
}

img {
Copy link

Choose a reason for hiding this comment

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

use class selector for image styles

src/style.css
}

img {
position: absolute;
Copy link

Choose a reason for hiding this comment

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

position absolute seems redundant here.

Copy link

Choose a reason for hiding this comment

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

Not fixed
image

src/style.css Outdated
Comment on lines 33 to 40
.container {
display: flex;
width: 1200px;
height: 60px;
justify-content: flex-end;

align-items: center;
}
Copy link

Choose a reason for hiding this comment

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

Move styles to header instead, and remove width and height properties.

src/style.css Outdated
border-radius: 8px; /* Закруглення країв лінії */
}

nav a {
Copy link

Choose a reason for hiding this comment

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

use class selector here as well.

@yurzxw yurzxw requested a review from vadiimvooo October 19, 2024 12:54
volodymyr-soltys97
volodymyr-soltys97 suggested changes Oct 19, 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 remove padding left for ul for fix failed tests
image

The height for the links must be 60px
image

src/index.html Outdated
</a>
<nav class="nav">
<ul class="nav_list">
<div class="underline">
Copy link

Choose a reason for hiding this comment

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

This div is redundant, you need to add line with ::after

Suggested change
<div class="underline">

src/index.html Outdated
<li class="nav_item">
<a
href="#"
class="nav_link blue is-active"
Copy link

Choose a reason for hiding this comment

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

Suggested change
class="nav_link blue is-active"
class="nav_link is-active"

src/index.html Outdated
Comment on lines 59 to 60
</li>
<li class="nav_item">
Copy link

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
@@ -1,3 +1,62 @@
body {
margin: 0;
font-size: 12px;
font-family: Roboto, sans-serif;
Copy link

Choose a reason for hiding this comment

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

Not fixed
image

src/style.css
@@ -1,3 +1,64 @@
body {
margin: 0;
font-family: Roboto, sans-serif;

--hover-color: #00acdc;
Copy link

Choose a reason for hiding this comment

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

Create a variable with :root selector

src/style.css
}

img {
position: absolute;
Copy link

Choose a reason for hiding this comment

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

Not fixed
image

volodymyr-soltys97
volodymyr-soltys97 reviewed Oct 19, 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.

remove this class

src/index.html Outdated
<li class="nav_item">
<a
href="#"
class="nav_link zero"
Copy link

Choose a reason for hiding this comment

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

Suggested change
class="nav_link zero"
class="nav_link"

vadiimvooo
vadiimvooo approved these changes Oct 20, 2024
View reviewed changes
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.

LGTM

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

@vadiimvooo vadiimvooo vadiimvooo 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
@yurzxw @volodymyr-soltys97 @vadiimvooo