Develop #5924
Develop #5924
Conversation
winzzzton
commented
Jan 10, 2025
•
winzzzton
commented
- 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 ❗️
There was a problem hiding this comment.
Great job on your implementation! 🎉 Your code meets most of the task requirements and constraints, and the functionality is intact. While there are a few minor issues, such as the use of the universal selector * and the need for a more descriptive alt attribute for the logo image, these do not significantly impact the functionality of your code. Keep up the good work, and consider these suggestions for future improvements! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/style.css
Outdated
| @@ -1,3 +1,79 @@ | |||
| body { | |||
| :root { | |||
| --blue-color: #00acdc | |||
There was a problem hiding this comment.
It seems like there is a missing semicolon at the end of the CSS variable declaration --blue-color: #00acdc. Adding a semicolon will ensure proper CSS syntax.
src/index.html
Outdated
| </a> | ||
| <nav class="menu"> | ||
| <ul class="nav"> | ||
| <li><a href="#" class="nav-link is-active">Apple</a></li> |
There was a problem hiding this comment.
| <li><a href="#" class="nav-link is-active">Apple</a></li> | |
| <li> | |
| <a | |
| href="#" | |
| class="nav-link is-active" | |
| > | |
| Apple | |
| </a> | |
| </li> |
There was a problem hiding this comment.
Please refactor your code according to styleguide
src/style.css
Outdated
| @@ -1,3 +1,79 @@ | |||
| body { | |||
| :root { | |||
| --blue-color: #00acdc | |||
src/style.css
Outdated
|
|
||
| header { | ||
| width: 100%; | ||
| height: 60px; |
There was a problem hiding this comment.
Height should not be hardcoded to header, instead it should be used for links
src/style.css
Outdated
| font-family: 'Roboto', sans-serif; | ||
| } | ||
|
|
||
| header { |
There was a problem hiding this comment.
Use class selector, the same for other selectors
src/style.css
Outdated
| .logo { | ||
| width: 40px; | ||
| height: 40px; | ||
| margin-left: 50px; |
There was a problem hiding this comment.
Instead of margin for this element use padding for header
src/style.css
Outdated
| width: 40px; | ||
| height: 40px; | ||
| margin-left: 50px; | ||
| margin-top: 10px; |
There was a problem hiding this comment.
Just use align-items cented to center element vertically inside header
