Develop #5372
Develop #5372
Conversation
There was a problem hiding this comment.
Great effort on the work you've done! However, we need to ensure everything is in top shape before we can approve it. There are a few important aspects to address, such as ensuring proper class naming for clarity, adding a fallback for the font-family, and fixing the indentation and formatting issues for better code readability. Keep up the good work, and with these adjustments, your code will be even better! 👍💻
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/index.html
Outdated
| <li> | ||
| <a | ||
| href="#" | ||
| class="nav__item is-active" | ||
| > | ||
| Apple | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| href="#" | ||
| class="nav__item" | ||
| > | ||
| Samsung | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| href="#" | ||
| class="nav__item" | ||
| > | ||
| Smartphones | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| href="#" | ||
| data-qa="hover" | ||
| class="nav__item" | ||
| > | ||
| Laptops & Computers | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| href="#" | ||
| class="nav__item" | ||
| > | ||
| Gadgets | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| href="#" | ||
| class="nav__item" | ||
| > | ||
| Tablets | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| href="#" | ||
| class="nav__item" | ||
| > | ||
| Photo | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| href="#" | ||
| class="nav__item" | ||
| > | ||
| Video | ||
| </a> | ||
| </li> |
There was a problem hiding this comment.
Add empty lines between multiline sibling blocks of HTML. There should be empty lines between the <li> elements to make the code more readable.
| class="nav__item is-active" | ||
| > | ||
| Apple | ||
| </a> |
There was a problem hiding this comment.
Class names represent the meaning of the content. The class name 'is-active' on line 49 could be more descriptive of its purpose, such as 'nav__item--active'.
src/style.css
Outdated
| } | ||
|
|
||
| html { | ||
| font-family: Roboto, sans-serif; |
There was a problem hiding this comment.
Consider adding a fallback font-family for Roboto in case it fails to load. For example, font-family: Roboto, Arial, sans-serif; ensures that there will be a similar font displayed if Roboto is not available.
| .nav { | ||
| margin: 0 50px 0 0; | ||
| } |
There was a problem hiding this comment.
Avoid using specific side margins for elements like .nav. It can lead to unexpected layouts on different screen sizes. Consider using padding within the parent element or CSS Grid/Flexbox properties for spacing.
| li { | ||
| margin-right: 20px; | ||
| } |
There was a problem hiding this comment.
Styling using tag names can cause unwanted side effects if the same tag is used in a different context. Use class names to apply styles to specific elements. Replace the li selector with a class selector, for example .nav__item.
DEMO LINK
TEST REPORT LINK
[ x] Header height is set in 1 place (for the links)
[ x] 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 ❗️