Develop #5728
Develop #5728
Conversation
IvanIlnytskyy
commented
Nov 25, 2024
IvanIlnytskyy
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Great job on your submission! 🎉 While there are a few minor issues, such as the CSS variable for the blue color and some commented-out code, these don't significantly impact the functionality. Keep up the good work and consider the feedback for future improvements! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/index.html
Outdated
| <li class="nav__item"> | ||
| <a | ||
| href="#" | ||
| class="nav__link nav__items" |
There was a problem hiding this comment.
The class 'nav__items' seems to be a typo or unnecessary since it is not used elsewhere in the code. Consider removing it or ensuring it is used consistently if intended.
| <a | ||
| href="#" | ||
| class="nav__link nav__items" | ||
| data-qa="hover" |
There was a problem hiding this comment.
The 'data-qa' attribute is not explained or used in the code. If it's not needed, consider removing it to avoid confusion. If it serves a purpose, ensure it is documented or utilized properly.
src/style.css
Outdated
| font-weight: 500; | ||
| min-width: 1024px; | ||
| position: relative; | ||
| box-shadow: 0 2px 4px rgba(0, 0, 0, 0, 5); |
There was a problem hiding this comment.
The box-shadow property has an incorrect value 'rgba(0, 0, 0, 0, 5)'. The last parameter should be a decimal between 0 and 1 to represent opacity. Consider correcting this to achieve the desired shadow effect.
src/style.css
Outdated
| /* .nav__link:hover { | ||
| text-decoration: underline; | ||
| text-decoration-color: #38a1f6; | ||
| text-decoration-thickness: 2px; | ||
| } */ |
There was a problem hiding this comment.
The hover effect for '.nav__link' is commented out. If this effect is intended to be part of the design, consider uncommenting and adjusting it to fit the overall style.
src/style.css
Outdated
| display: block; | ||
| text-align: center; | ||
| font-size: 12px; | ||
| margin-right: 20px; |
There was a problem hiding this comment.
set margin right for nav__item. do not set margin-right for last nav__item. it should help to fix tests
There was a problem hiding this comment.
видалив з останнього li клас nav__item, правий марджин 20пкс замість nav__link поставив для класу nav__item. дві помилки зникли ще дві залишились
| .nav__list:first-child { | ||
| margin-left: 0; | ||
| } | ||
|
|
||
| .nav__list:last-child { | ||
| margin-right: 0; | ||
| } |
There was a problem hiding this comment.
| .nav__list:first-child { | |
| margin-left: 0; | |
| } | |
| .nav__list:last-child { | |
| margin-right: 0; | |
| } |
looks like redundant
src/index.html
Outdated
| <link | ||
| rel="stylesheet" | ||
| href="./style.css" | ||
| /> | ||
| </head> | ||
| <body> | ||
| <h1>Moyo header</h1> | ||
| <header> |
There was a problem hiding this comment.
| <header> | |
| <header class="header"> |
src/index.html
Outdated
| </a> | ||
| <nav class="nav"> |
There was a problem hiding this comment.
| </a> | |
| <nav class="nav"> | |
| </a> | |
| <nav class="nav"> |
src/index.html
Outdated
| </li> | ||
| <li class="nav__item"> |
There was a problem hiding this comment.
Add the blank line between the links
| </li> | |
| <li class="nav__item"> | |
| </li> | |
| <li class="nav__item"> |
src/style.css
Outdated
| @@ -1,3 +1,75 @@ | |||
| body { | |||
| margin: 0; | |||
| padding: 0; | |||
There was a problem hiding this comment.
The body hasn't a padding
| padding: 0; |
src/style.css
Outdated
| .logo img { | ||
| max-height: 40px; | ||
| margin-left: 50px; | ||
| margin-top: 10px; | ||
| margin-bottom: 10px; | ||
| } |
There was a problem hiding this comment.
| .logo img { | |
| max-height: 40px; | |
| margin-left: 50px; | |
| margin-top: 10px; | |
| margin-bottom: 10px; | |
| } | |
| .logo { | |
| height: 40px; | |
| width: 40px; | |
| } |
src/style.css
Outdated
| min-width: 1024px; | ||
| position: relative; |
There was a problem hiding this comment.
Don't need to use position here
| min-width: 1024px; | |
| position: relative; | |
| width: 100%; |
src/style.css
Outdated
| font-family: Roboto, sans-serif; | ||
| margin-right: 0; | ||
| } |
There was a problem hiding this comment.
| font-family: Roboto, sans-serif; | |
| margin-right: 0; | |
| } | |
| font-family: Roboto, sans-serif; | |
| padding: 0 50px; | |
| box-sizing: border-box; | |
| } |
| } | ||
|
|
||
| .nav__link.is-active { | ||
| color: #00acdc; |
There was a problem hiding this comment.
Create a variable for repeated color and use it everywhere
src/style.css
Outdated
| margin-right: 0; | ||
| } | ||
|
|
||
| [data-qa='hover'].nav__link:hover { |
There was a problem hiding this comment.
| [data-qa='hover'].nav__link:hover { | |
| .nav__link:hover { |
src/style.css
Outdated
| color: #00acdc; | ||
| } | ||
|
|
||
| .nav__link.is-active::after { |
There was a problem hiding this comment.
| .nav__link.is-active::after { | |
| .is-active::after { |
