Develop #5391
Develop #5391
Conversation
taniavozniuk
commented
Sep 20, 2024
taniavozniuk
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Great job on structuring your HTML! 🌟 While there are some improvements to be made regarding best practices, they don't critically impact the functionality or security of your code. Keep an eye on styling conventions and indentation for future projects to enhance code readability and maintainability! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <link | ||
| rel="preconnect" | ||
| href="https://fonts.gstatic.com" | ||
| /> |
There was a problem hiding this comment.
The attributes in the tag should start each on a new line with 2-space indentation related to the tag to keep the attributes correctly formatted.
src/index.html
Outdated
| <header class="header"> | ||
| <a href="https://www.moyo.ua/ua/"> | ||
| <img | ||
| src="images/logo.png" | ||
| alt="Moyo logo" | ||
| class="logo" | ||
| /> | ||
| </a> | ||
| <nav> | ||
| <ul class="nav"> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/portal_brand/apple_1/" | ||
| class="nav__link is-active" | ||
| > | ||
| Apple | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/telecommunication/smart/samsung/" | ||
| class="nav__link" | ||
| > | ||
| Samsung | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/comp-and-periphery/" | ||
| class="nav__link" | ||
| > | ||
| Smartphones | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/comp-and-periphery/" | ||
| class="nav__link" | ||
| data-qa="hover" | ||
| > | ||
| Laptops & Computers | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/gadgets/" | ||
| class="nav__link" | ||
| > | ||
| Gadgets | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/tablet_el_knigi/tablet/" | ||
| class="nav__link" | ||
| > | ||
| Tablets | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/foto_video/photo_video/" | ||
| class="nav__link" | ||
| > | ||
| Photo | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/foto_video/photo_video/" | ||
| class="nav__link" | ||
| > | ||
| Video | ||
| </a> | ||
| </li> | ||
| </ul> | ||
| </nav> | ||
| </header> |
There was a problem hiding this comment.
Add empty lines between multiline sibling blocks of HTML to improve readability. For example, add an empty line before the
element.
src/index.html
Outdated
| <ul class="nav"> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/portal_brand/apple_1/" | ||
| class="nav__link is-active" | ||
| > | ||
| Apple | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/telecommunication/smart/samsung/" | ||
| class="nav__link" | ||
| > | ||
| Samsung | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/comp-and-periphery/" | ||
| class="nav__link" | ||
| > | ||
| Smartphones | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/comp-and-periphery/" | ||
| class="nav__link" | ||
| data-qa="hover" | ||
| > | ||
| Laptops & Computers | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/gadgets/" | ||
| class="nav__link" | ||
| > | ||
| Gadgets | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/tablet_el_knigi/tablet/" | ||
| class="nav__link" | ||
| > | ||
| Tablets | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/foto_video/photo_video/" | ||
| class="nav__link" | ||
| > | ||
| Photo | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/foto_video/photo_video/" | ||
| class="nav__link" | ||
| > | ||
| Video | ||
| </a> | ||
| </li> | ||
| </ul> |
There was a problem hiding this comment.
Remember about correct indentation between parent and child elements. The
- element.
src/index.html
Outdated
| <a | ||
| href="https://www.moyo.ua/ua/portal_brand/apple_1/" | ||
| class="nav__link is-active" | ||
| > |
There was a problem hiding this comment.
The href attribute in the tag should not contain spaces. Instead, use a valid URL or a path that does not include spaces.
| *, | ||
| *::before, | ||
| *::after { | ||
| box-sizing: border-box; | ||
| } |
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 browser.
| body { | ||
| margin: 0; | ||
| padding: 0; | ||
| font-family: Roboto, Arial, 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; should have a generic family name as a fallback, like sans-serif.
| ul { | ||
| margin: 0; | ||
| padding: 0; | ||
| list-style-type: none; | ||
| display: flex; | ||
| } |
There was a problem hiding this comment.
Don't use tag names for styling (except html and body). Style all elements using .class and if needed with :pseudo-class, pseudo-element and [attribute]. For example, instead of styling ul, use .nav__list.
src/style.css
Outdated
| font-family: Roboto, Arial, sans-serif; | ||
| } | ||
|
|
||
| header { |
There was a problem hiding this comment.
Use only class selector
| header { | |
| .header { |
src/index.html
Outdated
| <ul class="nav"> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/portal_brand/apple_1/" |
There was a problem hiding this comment.
You can fix href everywhere
| href="https://www.moyo.ua/ua/portal_brand/apple_1/" | |
| href="#apple" |
src/style.css
Outdated
| } | ||
|
|
||
| header { | ||
| background-color: #fff; |
There was a problem hiding this comment.
This color is default
| background-color: #fff; |
src/style.css
Outdated
|
|
||
| header { | ||
| background-color: #fff; | ||
| justify-content: space-between; /* Розподіляє елементи по сторонах */ |
There was a problem hiding this comment.
Remove all comments, it's redundant here
| justify-content: space-between; /* Розподіляє елементи по сторонах */ | |
| justify-content: space-between; |
src/style.css
Outdated
| } | ||
|
|
||
| .nav__link:hover { | ||
| color: #00acdc; |
There was a problem hiding this comment.
Create a variable for repeated color and use it everywhere
|
|
||
| .is-active { | ||
| position: relative; | ||
| color: #00acdc; |
There was a problem hiding this comment.
| color: #00acdc; | |
| color: var(--primary-color); |
