add task solution #4831
add task solution #4831
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! 😊 While your code is mostly well-structured and meets many of the task requirements, there are a few critical issues that need to be addressed before we can approve the pull request:
-
SCSS Selector Issues: There are some incorrect SCSS selectors, such as the extra space in the modifier class (
'&--1 :first-child') and the hover effect selector ('&:hover &__title'). These need to be corrected to ensure the styles apply as intended. -
Navigation and Button Links: All navigation links currently point to
'index.html', and the 'Buy' button'shrefis set to'#'. These should be updated to point to the correct pages or actions to ensure proper navigation and functionality. -
Margin Inheritance: Using
'margin: inherit;'might not work as expected if the parent element doesn't have a defined margin. Consider specifying a concrete value or removing it if not necessary.
Please address these issues to improve the functionality and maintainability of your code. Once these changes are made, your code will be in great shape for approval! Keep up the great 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 | ||
| class="nav__link is-active" | ||
| href="index.html" |
There was a problem hiding this comment.
All navigation links point to 'index.html'. If these links are intended to navigate to different pages, ensure the 'href' attributes are updated accordingly.
| <a | ||
| class="nav__link" | ||
| href="index.html" |
There was a problem hiding this comment.
All navigation links point to 'index.html'. If these links are intended to navigate to different pages, ensure the 'href' attributes are updated accordingly.
| <a | ||
| class="nav__link" | ||
| href="index.html" |
There was a problem hiding this comment.
All navigation links point to 'index.html'. If these links are intended to navigate to different pages, ensure the 'href' attributes are updated accordingly.
| <a | ||
| class="nav__link" | ||
| href="index.html" |
There was a problem hiding this comment.
All navigation links point to 'index.html'. If these links are intended to navigate to different pages, ensure the 'href' attributes are updated accordingly.
| <a | ||
| class="nav__link" | ||
| href="index.html" |
There was a problem hiding this comment.
All navigation links point to 'index.html'. If these links are intended to navigate to different pages, ensure the 'href' attributes are updated accordingly.
| <a | ||
| href="#" | ||
| class="card__button" |
There was a problem hiding this comment.
The 'href' attribute of the 'Buy' button is set to '#', which means it doesn't lead anywhere. Consider updating this to point to a relevant page or action.
| <a | ||
| href="#" | ||
| class="card__button" |
There was a problem hiding this comment.
The 'href' attribute of the 'Buy' button is set to '#', which means it doesn't lead anywhere. Consider updating this to point to a relevant page or action.
src/styles/blocks/card.scss
Outdated
| &:hover &__title { | ||
| color: #34568b; | ||
| } |
There was a problem hiding this comment.
The selector '&:hover &__title' is incorrect. The '&' in SCSS refers to the parent selector, so this will not apply the hover effect as intended. You should use '.card:hover .card__title' instead to change the title color on hover.
src/styles/blocks/card.scss
Outdated
| margin: inherit; | ||
| margin-bottom: 16px; |
There was a problem hiding this comment.
Using 'margin: inherit;' might not work as expected if the parent element doesn't have a defined margin. Consider specifying a concrete value or removing it if not necessary.
| &--1 :first-child, | ||
| &--2 :nth-child(-n + 2), | ||
| &--3 :nth-child(-n + 3), | ||
| &--4 :nth-child(-n + 4), | ||
| &--5 :nth-child(-n + 5) { |
There was a problem hiding this comment.
There is an extra space after the modifier class (e.g., '&--1 :first-child'). This space can cause the selector to target any child element rather than the intended direct child. Remove the space to ensure the selector targets the correct elements (e.g., '&--1:first-child').
There was a problem hiding this comment.
Your code meets most of the task requirements, but there are a few issues to address for improvement. The HTML structure is correct, but ensure that the href attributes in navigation links and 'Buy' buttons point to the correct URLs. The SCSS files are well-structured, but be cautious with hover effects that cause layout shifts and ensure all variables are defined to avoid compilation errors. The checklist file should include verified items as specified. Address these issues to enhance your code quality. Keep up the good work and continue learning from these insights! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <li> | ||
| <a | ||
| class="nav__link is-active" | ||
| href="index.html" |
There was a problem hiding this comment.
The 'href' attribute for this navigation link is set to 'index.html'. Ensure this is the correct link for the 'Apple' section. If not, update it to the correct URL.
| <li> | ||
| <a | ||
| class="nav__link" | ||
| href="index.html" |
There was a problem hiding this comment.
The 'href' attribute for this navigation link is set to 'index.html'. Ensure this is the correct link for the 'Samsung' section. If not, update it to the correct URL.
| <li> | ||
| <a | ||
| class="nav__link" | ||
| href="index.html" |
There was a problem hiding this comment.
The 'href' attribute for this navigation link is set to 'index.html'. Ensure this is the correct link for the 'Smartphones' section. If not, update it to the correct URL.
| <li> | ||
| <a | ||
| class="nav__link" | ||
| href="index.html" |
There was a problem hiding this comment.
The 'href' attribute for this navigation link is set to 'index.html'. Ensure this is the correct link for the 'Laptops & Computers' section. If not, update it to the correct URL.
| <li> | ||
| <a | ||
| class="nav__link" | ||
| href="index.html" |
There was a problem hiding this comment.
The 'href' attribute for this navigation link is set to 'index.html'. Ensure this is the correct link for the 'Gadgets' section. If not, update it to the correct URL.
| } | ||
|
|
||
| &:visited { | ||
| color: $main-text-color; |
There was a problem hiding this comment.
Ensure that the variable $main-text-color is defined in your SCSS files. If not, define it to avoid compilation errors.
| } | ||
|
|
||
| &:hover { | ||
| color: $blue-color; |
There was a problem hiding this comment.
Ensure that the variable $blue-color is defined in your SCSS files. If not, define it to avoid compilation errors.
|
|
||
| &::after { | ||
| content: ''; | ||
| background-color: $blue-color; |
There was a problem hiding this comment.
Ensure that the variable $blue-color is defined in your SCSS files. If not, define it to avoid compilation errors.
| } | ||
|
|
||
| &:link { | ||
| color: $blue-color; |
There was a problem hiding this comment.
Ensure that the variable $blue-color is defined in your SCSS files. If not, define it to avoid compilation errors.
| } | ||
|
|
||
| &:visited { | ||
| color: $blue-color; |
There was a problem hiding this comment.
Ensure that the variable $blue-color is defined in your SCSS files. If not, define it to avoid compilation errors.
DEMO LINK
TEST REPORT LINK
All components follow BEM and use SCSS
Repeated 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
:hoverare smoothCode follows all the Code Style Rules ❗️