add task solution #5378
add task solution #5378
Conversation
miroshnykow
commented
Sep 17, 2024
miroshnykow
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Great job on the Pull Request! 🌟 There are a few minor issues pointed out, such as styling with tag names, missing fallback fonts, and using the * selector, which can be improved in future work. However, none of these issues are critical enough to warrant a request for changes. Keep up the good work, and consider these tips to enhance your code quality for the next time! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <header class="header"> | ||
| <a | ||
| class="logo" | ||
| href="#" | ||
| > | ||
| <img | ||
| src="/src/images/logo.png" | ||
| alt="logo" | ||
| /> | ||
| </a> |
There was a problem hiding this comment.
Remember about correct indentation between parent and child elements. The img element should have a 2-space offset from the a element.
| <img | ||
| src="/src/images/logo.png" | ||
| alt="logo" |
There was a problem hiding this comment.
The alt attribute should describe the image content, not just say 'logo'. It should ideally describe what the logo is about or what company it represents.
| <ul class="nav__list"> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="#" | ||
| class="nav__link is-active" | ||
| > | ||
| Apple | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="#" | ||
| class="nav__link" | ||
| > | ||
| Samsung | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="#" | ||
| class="nav__link" | ||
| > | ||
| Smartphones | ||
| </a> | ||
| </li> | ||
| <li | ||
| class="nav__item" | ||
| data-qa="hover" | ||
| > | ||
| <a | ||
| href="#" | ||
| class="nav__link" | ||
| > | ||
| Laptops & Computers | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="#" | ||
| class="nav__link" | ||
| > | ||
| Gadgets | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="#" | ||
| class="nav__link" | ||
| > | ||
| Tablets | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="#" | ||
| class="nav__link" | ||
| > | ||
| Photo | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="#" | ||
| class="nav__link" | ||
| > | ||
| Video | ||
| </a> | ||
| </li> | ||
| </ul> |
There was a problem hiding this comment.
Add empty lines between multiline sibling blocks of HTML. There should be an empty line between each li element to improve readability.
| <a | ||
| href="#" | ||
| class="nav__link is-active" | ||
| > | ||
| Apple |
There was a problem hiding this comment.
Don't use spaces in <a> tag's href property. If these are placeholder links, consider using # without any following space.
| * { | ||
| 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.
| display: flex; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
| font-family: Roboto, system-ui; |
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, 'Roboto', Arial, sans-serif.
| .nav__item { | ||
| list-style: none; |
There was a problem hiding this comment.
Don't use tag names for styling (except html and body). Style all elements using .class. Also, the .nav__item class already sets list-style: none;, so it's redundant to declare it here again.
src/style.css
Outdated
| border-radius: 2px; | ||
| } | ||
|
|
||
| .nav__item[data-qa='hover']:hover .nav__link { |
There was a problem hiding this comment.
| .nav__item[data-qa='hover']:hover .nav__link { | |
| .nav__link::hover { |
src/style.css
Outdated
| .nav__item[data-qa='hover']:hover .nav__link { | ||
| color: #00acdc; | ||
| } | ||
| /* stylelint-disable-next-line rule-empty-line-before */ |
There was a problem hiding this comment.
| /* stylelint-disable-next-line rule-empty-line-before */ |
src/style.css
Outdated
| } | ||
|
|
||
| .is-active { | ||
| color: #00acdc; |
There was a problem hiding this comment.
Consider using variables for repeating colors
