Moyo Header #5376
Moyo Header #5376
Conversation
rssydorenko
commented
Sep 17, 2024
rssydorenko
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Great job on your Pull Request! You've made some good progress. Just a few minor things to keep in mind for the future: try to stick to BEM naming conventions for your classes, remember to include fallback fonts, avoid using the * selector for better performance, and watch out for spaces in your href attributes. Keep up the good work! 👍🎉
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/index.html
Outdated
| </a> | ||
|
|
||
| <nav class="nav"> | ||
| <ul class="nav_list"> |
There was a problem hiding this comment.
Class name 'nav_list' should be 'nav__list' to maintain BEM naming conventions which represent the meaning of the content and not the styles or tag names.
src/index.html
Outdated
|
|
||
| <nav class="nav"> | ||
| <ul class="nav_list"> | ||
| <li class="nav_item"> |
There was a problem hiding this comment.
Class name 'nav_item' should be 'nav__item' to maintain BEM naming conventions which represent the meaning of the content and not the styles or tag names.
src/index.html
Outdated
| <a | ||
| href="#apple" | ||
| class="nav_link is-active" | ||
| > |
There was a problem hiding this comment.
Class name 'nav_link' should be 'nav__link' to maintain BEM naming conventions which represent the meaning of the content and not the styles or tag names.
src/index.html
Outdated
| href="#laptops-computers" | ||
| class="nav_link" | ||
| data-qa="hover" | ||
| > | ||
| Laptops & Computers |
There was a problem hiding this comment.
Don't use spaces in the <a> tag's href property. The correct value should be '#laptops-computers' without the space.
src/style.css
Outdated
| * { | ||
| margin: 0; | ||
| padding: 0; | ||
| box-sizing: border-box; | ||
| list-style: none; |
There was a problem hiding this comment.
Don't use * selector (it impacts performance). Set styles only for elements that require them. Zeroing out your margins, paddings or other styles with '*' is still inefficient for the browser.
src/style.css
Outdated
|
|
||
| html { | ||
| font-size: 16px; | ||
| font-family: Roboto, sans-serif; |
There was a problem hiding this comment.
Remember to use fallback fonts - alternative font-family in case the main one doesn't work. For example, font-family: Roboto, Arial, sans-serif;.
src/style.css
Outdated
| width: 40px; | ||
| } | ||
|
|
||
| .nav_list { |
There was a problem hiding this comment.
Class names should represent the meaning of the content, not the styles or tag names. Here, .nav_list should be .nav__list to follow the BEM naming convention.
src/style.css
Outdated
| max-width: 1200px; | ||
| margin: 0 auto; |
There was a problem hiding this comment.
| max-width: 1200px; | |
| margin: 0 auto; | |
| width: 100%; | |
| box-sizing: border-box; |
src/style.css
Outdated
| } | ||
|
|
||
| .nav_item:hover { | ||
| color: #00acdc; |
There was a problem hiding this comment.
Create a variable for repeated color and use it everywhere
