Develop 1 #5353
Develop 1 #5353
Conversation
AndriiKasiian
commented
Sep 12, 2024
AndriiKasiian
commented
- DEMO LINK
- TEST REPORT LINK
-
- Header height is set in 1 place (for the links)
- Content is vertically centered (for any header height)
- [x ] CSS is used to show all letters in Uppercase (don't type them in HTML)
- [x ] Logo is an image wrapped with a link
- [x ] CSS Variable is used for a blue color
- [ x] Pseudo-element is used for a blue line below the active link
- [ x] Code follows all the Code Style Rules ❗️
There was a problem hiding this comment.
Hey there! Great work on your Pull Request. 👍 I noticed a few important things that need your attention. Please make sure to use 'min-width' or 'max-width' in your media queries for better responsiveness, and avoid using tag names for styling. Also, ensure that container sizes are not fixed unless required, and always provide fallback fonts. Lastly, keep an eye on the character limit for lines and the use of BEM naming conventions for your classes. Let's make these changes for a cleaner and more efficient code! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <a | ||
| class="logo" | ||
| href="index.html" | ||
| > | ||
| <img | ||
| src="/src/images/logo.png" | ||
| alt="moyo" | ||
| /> |
There was a problem hiding this comment.
The href attribute in the tag should not contain spaces. Consider changing href="index.html" to href="#home" or another appropriate anchor link.
| > | ||
| <img | ||
| src="/src/images/logo.png" | ||
| alt="moyo" |
There was a problem hiding this comment.
The alt attribute for the logo image should be more descriptive about the content. For example, alt="Moyo Logo".
| <nav class="nav"> | ||
| <ul class="ul__nav"> | ||
| <li class="li__nav"> | ||
| <a | ||
| href="#section-1" | ||
| class="nav__link is-active" | ||
| > | ||
| APPLE | ||
| </a> | ||
| </li> | ||
| <li class="li__nav"> | ||
| <a | ||
| href="#section-2" | ||
| class="nav__link" | ||
| > | ||
| SAMSUNG | ||
| </a> | ||
| </li> | ||
| <li class="li__nav"> | ||
| <a | ||
| href="#section-3" | ||
| class="nav__link" | ||
| > | ||
| SMARTPHONES | ||
| </a> | ||
| </li> | ||
| <li class="li__nav"> | ||
| <a | ||
| href="#section-4" | ||
| data-qa="hover" | ||
| class="nav__link laptops" | ||
| > | ||
| Laptops & Computers | ||
| </a> | ||
| </li> | ||
| <li class="li__nav"> | ||
| <a | ||
| href="#section-5" | ||
| class="nav__link" | ||
| > | ||
| Gadgets | ||
| </a> | ||
| </li> | ||
| <li class="li__nav"> | ||
| <a | ||
| href="#section-6" | ||
| class="nav__link" | ||
| > | ||
| Tablets | ||
| </a> | ||
| </li> | ||
| <li class="li__nav"> | ||
| <a | ||
| href="#section-7" | ||
| class="nav__link" | ||
| > | ||
| Photo | ||
| </a> | ||
| </li> | ||
| <li class="li__nav"> | ||
| <a | ||
| href="#section-8" | ||
| class="nav__link" | ||
| > | ||
| Video | ||
| </a> | ||
| </li> | ||
| </ul> |
There was a problem hiding this comment.
Class names should represent the meaning of the content, not the styles or tag names. For example, ul__nav should be nav__list, and li__nav should be nav__item.
| href="#section-4" | ||
| data-qa="hover" | ||
| class="nav__link laptops" | ||
| > |
There was a problem hiding this comment.
The class laptops does not follow the BEM naming convention. It should be something like nav__link--laptops if it's a modifier.
| data-qa="hover" | ||
| class="nav__link laptops" | ||
| > | ||
| Laptops & Computers |
There was a problem hiding this comment.
The text inside the tag is too long and exceeds the maximum of 80 characters per line. Consider breaking it into multiple lines.
| @@ -1,3 +1,106 @@ | |||
| html { | |||
| margin: 0; | |||
| 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 instance, you could specify a generic font family as a fallback like 'sans-serif'.
| display: flex; | ||
| position: relative; | ||
| align-items: center; | ||
| width: 1024px; |
There was a problem hiding this comment.
Don't fix container size (if there is no such a requirement). Let the content size dictate it to avoid overflow or accidental scroll bar.
| .ul__nav { | ||
| list-style: none; | ||
| display: flex; | ||
| margin: 0; | ||
| padding: 0; |
There was a problem hiding this comment.
Don't use tag names for styling (except 'html' and 'body'). Style all elements using '.class'. Instead of .ul__nav, use a class that describes the role or content of the list, such as .nav__list.
| .li__nav { | ||
| display: flex; | ||
| margin-right: 20px; | ||
|
|
||
| position: relative; |
There was a problem hiding this comment.
Don't use tag names for styling (except 'html' and 'body'). Style all elements using '.class'. Instead of .li__nav, use a class that describes the role or content of the list item, such as .nav__item.
| @media (width: 1200px) { | ||
| .container { | ||
| width: 1200px; | ||
| } |
There was a problem hiding this comment.
Media queries should use 'min-width' or 'max-width' rather than 'width' for better responsiveness across different screen sizes.
