add task solution #4797
add task solution #4797
Conversation
yurzxw
commented
Oct 23, 2024
•
yurzxw
commented
- DEMO LINK
- TEST REPORT LINK
- All component follow BEM and use SCSS
- repaeted sizes and special colors are put to variables
- Grid is used for the columns
- cards are shown in 1, 2, 3 or 4 columns based on screen resolution
- All changes on :hover are smooth
- Code follows all the Code Style Rules ❗️
There was a problem hiding this comment.
There are several critical issues that need to be addressed, including a syntax error in the grid layout, potential layout issues with scaling and padding, and an undefined mixin. Please resolve these to ensure the functionality and maintainability of the code. Keep up the good work! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.
| <a | ||
| href="#" | ||
| class="moyo" | ||
| > | ||
| <img | ||
| src="./images/logo.png" | ||
| alt="Moyo" | ||
| /> | ||
| </a> |
There was a problem hiding this comment.
The anchor tag has an href attribute set to '#', which might not be functional. Ensure that this is intentional or replace it with a valid URL if needed.
src/index.html
Outdated
| <a | ||
| href="#" | ||
| class="nav_item__link is-active" | ||
| > | ||
| Apple | ||
| </a> |
There was a problem hiding this comment.
The href attribute is set to '#', which might not be functional. Consider providing a valid URL or ensuring this is intentional for navigation purposes.
src/index.html
Outdated
| <a | ||
| href="#" | ||
| class="nav_item__link" | ||
| > | ||
| Samsung | ||
| </a> |
There was a problem hiding this comment.
The href attribute is set to '#', which might not be functional. Consider providing a valid URL or ensuring this is intentional for navigation purposes.
src/index.html
Outdated
| <a | ||
| href="#" | ||
| class="nav_item__link" | ||
| > | ||
| Smartphones | ||
| </a> |
There was a problem hiding this comment.
The href attribute is set to '#', which might not be functional. Consider providing a valid URL or ensuring this is intentional for navigation purposes.
src/index.html
Outdated
| <a | ||
| href="#" | ||
| class="nav_item__link" | ||
| data-qa="nav-hover" | ||
| > | ||
| Laptops & Computers | ||
| </a> |
There was a problem hiding this comment.
The href attribute is set to '#', which might not be functional. Consider providing a valid URL or ensuring this is intentional for navigation purposes.
src/index.html
Outdated
| <a | ||
| href="#" | ||
| class="buy" | ||
| > | ||
| buy | ||
| </a> |
There was a problem hiding this comment.
The href attribute is set to '#', which might not be functional. Consider providing a valid URL or ensuring this is intentional for navigation purposes.
src/styles/button.scss
Outdated
| width: 166px; | ||
| height: 40px; | ||
| background-color: #00acdc; | ||
| color: #fff; | ||
| text-decoration: none; | ||
| border-radius: 5px; | ||
| margin: 0 16px 16px; | ||
| padding: 12px 70px; |
There was a problem hiding this comment.
The width is set to 166px and padding is set to 12px 70px. This could cause layout issues as the total width including padding exceeds the specified width, potentially causing overflow or unexpected layout behavior. Consider adjusting the width or padding to ensure consistency.
| row-gap: 46px; | ||
| padding: 50px 40px; | ||
|
|
||
| @include on-all; |
There was a problem hiding this comment.
The '@include on-all;' directive is used here, but it's not clear if 'on-all' is defined elsewhere in your SCSS files. Ensure that 'on-all' is a valid mixin or remove this line if it's not needed.
src/styles/product_card.scss
Outdated
| &:hover { | ||
| transition: 0.3s; | ||
| transform: scale(1.2); |
There was a problem hiding this comment.
The transform: scale(1.2) on hover might cause layout issues by affecting the surrounding elements. Consider using a different approach or ensuring that the surrounding layout can accommodate this scaling without causing overflow or layout shifts.
src/styles/utils/mixins.scss
Outdated
| @@ -0,0 +1,21 @@ | |||
| @mixin on-all { | |||
| @media (min-width: 0) { | |||
| grid-template-columns: 1, 1fr; | |||
There was a problem hiding this comment.
There is a syntax error in 'grid-template-columns: 1, 1fr;'. The correct syntax should be 'grid-template-columns: repeat(1, 1fr);' to ensure proper grid layout.
src/index.html
Outdated
| @@ -22,6 +22,404 @@ | |||
| </head> | |||
|
|
|||
| <body> | |||
| <h1>Catalog</h1> | |||
| <div class="moyo_catalog"></div> | |||
There was a problem hiding this comment.
Remove it
| <div class="moyo_catalog"></div> |
src/index.html
Outdated
| <ul class="nav_list"> | ||
| <li class="nav_item"> | ||
| <a | ||
| href="#" | ||
| class="nav_item__link is-active" |
There was a problem hiding this comment.
Fix classes everywhere
| <ul class="nav_list"> | |
| <li class="nav_item"> | |
| <a | |
| href="#" | |
| class="nav_item__link is-active" | |
| <ul class="nav__list"> | |
| <li class="nav__item"> | |
| <a | |
| href="#" | |
| class="nav__link is-active" |
src/index.html
Outdated
| <div class="card_price"> | ||
| <p class="card_text">Price:</p> | ||
| <p class="card_dollars">$2,199</p> | ||
| </div> |
There was a problem hiding this comment.
| <div class="card_price"> | |
| <p class="card_text">Price:</p> | |
| <p class="card_dollars">$2,199</p> | |
| </div> | |
| <div class="card__price"> | |
| <p class="card__price--label">Price:</p> | |
| <p class="card__price--value">$2,199</p> | |
| </div> |
src/index.html
Outdated
| </div> | ||
| <a | ||
| href="#" | ||
| class="buy" |
There was a problem hiding this comment.
| class="buy" | |
| class="card__button" |
| </div> | ||
| <div |
There was a problem hiding this comment.
Add the blank line between the cards
| </div> | |
| <div | |
| </div> | |
| <div |
| justify-content: space-between; | ||
| } |
There was a problem hiding this comment.
| justify-content: space-between; | |
| } | |
| justify-content: space-between; | |
| padding: 0 50px; | |
| } |
src/styles/moyo_header.scss
Outdated
| .nav { | ||
| padding-right: 50px; | ||
| top: 0; | ||
| bottom: 0; | ||
| } |
There was a problem hiding this comment.
| .nav { | |
| padding-right: 50px; | |
| top: 0; | |
| bottom: 0; | |
| } |
| .moyo { | ||
| display: flex; | ||
| justify-content: center; | ||
| margin-left: 50px; |
There was a problem hiding this comment.
| margin-left: 50px; |
src/index.html
Outdated
| <li class="nav_item"> | ||
| <a | ||
| href="#" | ||
| class="nav_item__link z" |
There was a problem hiding this comment.
What is z?
| class="nav_item__link z" | |
| class="nav__link" |
src/styles/moyo_header.scss
Outdated
| .z { | ||
| margin-right: 0; | ||
| } |
There was a problem hiding this comment.
| .z { | |
| margin-right: 0; | |
| } |
| font-size: 14px; | ||
| font-weight: 700; | ||
| text-align: center; | ||
| width: 166px; |
| text-align: center; | ||
| width: 166px; | ||
| height: 40px; | ||
| background-color: #00acdc; |
src/styles/button.scss
Outdated
| transition: $transition-time; | ||
|
|
||
| &:hover { | ||
| background-color: #fff; |
There was a problem hiding this comment.
Consider creating a variable for the repeating color
| margin-right: 4px; | ||
| } | ||
|
|
||
| .stars--4 .stars__star:nth-child(-n + 4) { |
There was a problem hiding this comment.
Consider adding styles for other modifiers
src/styles/product_card.scss
Outdated
| border-radius: 5px; | ||
| box-sizing: border-box; | ||
| width: 200px; | ||
| height: 408px; |
There was a problem hiding this comment.
| height: 408px; |
| </div> | ||
| <div |
| font-size: 14px; | ||
| font-weight: 700; | ||
| text-align: center; | ||
| width: 166px; |
| text-align: center; | ||
| width: 166px; | ||
| height: 40px; | ||
| background-color: #00acdc; |
| color: var(--hover-color); | ||
| border: 1px solid var(--hover-color); |
| } | ||
|
|
||
| :root { | ||
| --hover-color: #00acdc; |
| justify-content: space-between; | ||
| } |
| .moyo { | ||
| display: flex; | ||
| justify-content: center; | ||
| margin-left: 50px; |
| margin-right: 4px; | ||
| } | ||
|
|
||
| .stars--4 .stars__star:nth-child(-n + 4) { |
