-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
task solution | second pull request #5347
base: master
Are you sure you want to change the base?
Conversation
kitaru64
commented
Sep 9, 2024
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls, feel free to ask for some help in the chat to fix your demo links
There was a problem hiding this 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
</li> | ||
<li> |
There was a problem hiding this comment.
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
</li> | |
<li> | |
</li> | |
<li> |
src/style.css
Outdated
margin: 0; | ||
box-sizing: border-box; | ||
font-family: Roboto, sans-serif; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move font family to the html selector
box-sizing: border-box; | ||
font-family: Roboto, sans-serif; | ||
|
||
--header-height: 60px; |
There was a problem hiding this comment.
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
Outdated
justify-content: space-between; | ||
align-items: center; | ||
|
||
position: fixed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant
position: fixed; |
src/index.html
Outdated
<nav class="navbar"> | ||
<ul class="navbar__list"> | ||
<li> | ||
<a | ||
href="#" | ||
class="navbar__link is-active" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix classes everywhere
<nav class="navbar"> | |
<ul class="navbar__list"> | |
<li> | |
<a | |
href="#" | |
class="navbar__link is-active" | |
<nav class="nav"> | |
<ul class="nav__list"> | |
<li class="nav__item"> | |
<a | |
href="#" | |
class="nav__link is-active" |
src/style.css
Outdated
margin: auto 0; | ||
} | ||
|
||
.navbar__list li:not(:first-child) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.navbar__list li:not(:first-child) { | |
.nav__item:not(:first-child) { |
src/style.css
Outdated
} | ||
|
||
.navbar__link:hover { | ||
color: #00acdc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create a variable for repeated color and use it everywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I almost sure that you need to update the color of links to pass tests
.nav__link:hover { | ||
color: var(--main-color); | ||
} | ||
|
||
a.is-active { | ||
color: var(--main-color); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.nav__link:hover { | |
color: var(--main-color); | |
} | |
a.is-active { | |
color: var(--main-color); | |
} | |
.nav__link:hover, | |
a.is-active { | |
color: var(--main-color); | |
} |