add product card #5030
add product card #5030
Conversation
mmromanova
commented
Dec 2, 2024
mmromanova
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Thank you for your hard work on this project! 😊 There are a few critical issues that need to be addressed to meet the core requirements of the task. Specifically, the BEM blocks need to be correctly named and separated into their own files, SCSS variables should be used and placed in a separate file, and the required attributes and hover styles should be implemented as specified. Once these are resolved, your solution will be much stronger. Keep up the great effort, and I'm confident you'll nail it with these adjustments! 🚀
✨ 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.
There was a problem hiding this comment.
Thank you for your submission! 🎉 While your code has been approved to help you move forward with your learning journey, there are several important points to address to ensure your code aligns with best practices and task requirements:
-
BEM Blocks: The task specifies the need for two separate BEM blocks,
cardandstars, each in their own file. Currently, your code uses a single BEM block namedcomp, which does not meet this requirement. Please ensure to separate these blocks in future implementations. -
SCSS Variables: It's great that you're using SCSS variables for colors and border-radius. However, ensure that all variables, such as
$blue_color,$white_color,$border-radius, and$font-family, are defined in a separatevariables.scssfile. This will help maintain consistency and make updates easier. -
BEM Naming Conventions: There are several issues with BEM naming:
- The classes
comp__price--1andcomp__price--2should be more descriptive, such ascomp__price--labelandcomp__price--value. - The class
comp__rate--stars--4should follow the single hyphen convention for modifiers, likecomp__rate--stars-4. - The class
comp__rate--stars__starshould be corrected tocomp__rate__starif it's an element, orcomp__rate--starif it's a modifier.
- The classes
-
Attributes and Hover Styles: Ensure that the
data-qa="card"attribute is added to the card block and that thedata-qa="hover"attribute is present on the linkBUYas required by the task. -
File Structure: The file structure should separate the
cardandstarsblocks into their own files, as specified in the task requirements.
Please review these comments and make the necessary adjustments in your future work. Addressing these issues will enhance the functionality and maintainability of your code. Keep up the good work and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/index.html
Outdated
| Product code: 195434 | ||
| </div> | ||
| <div class="comp__rate"> | ||
| <div class="comp__rate--stars comp__rate--stars--4"> |
There was a problem hiding this comment.
The class comp__rate--stars--4 is not following the BEM naming convention correctly. In BEM, modifiers should be separated by a single hyphen, not double. Consider renaming it to something like comp__rate--stars-4.
src/index.html
Outdated
| <div class="comp__rate--stars__star"></div> | ||
| <div class="comp__rate--stars__star"></div> | ||
| <div class="comp__rate--stars__star"></div> | ||
| <div class="comp__rate--stars__star"></div> | ||
| <div class="comp__rate--stars__star"></div> |
There was a problem hiding this comment.
The class comp__rate--stars__star is not correctly following BEM naming conventions. The double underscore is used for elements, and it should be comp__rate--star if it's a modifier, or comp__rate__star if it's an element of comp__rate.
src/index.html
Outdated
| <div class="comp__price--1"> | ||
| Price: | ||
| </div> | ||
| <div class="comp__price--2"> | ||
| $2,199 | ||
| </div> |
There was a problem hiding this comment.
The classes comp__price--1 and comp__price--2 do not clearly convey their purpose. In BEM, modifiers should describe the state or style of the block or element. Consider using more descriptive names, such as comp__price--label and comp__price--value.
| border: 1px solid $white_color; | ||
| border-radius: $border-radius; |
There was a problem hiding this comment.
Ensure that the variables $white_color and $border-radius are defined in a separate SCSS variables file, as per the task requirements. This helps in maintaining consistency and ease of updates across styles.
src/styles/button.scss
Outdated
| border: 1px solid $blue_color; | ||
| margin-top: 16px; | ||
| margin-left: 16px; | ||
| background-color: $blue_color; | ||
| color: $white_color; | ||
| text-align: center; | ||
| line-height: 40px; | ||
| height: 40px; | ||
| border-radius: $border-radius; | ||
| font-weight: 700; | ||
| font-size: 14px; | ||
| font-family: $font-family; |
There was a problem hiding this comment.
Ensure that the variables $blue_color, $white_color, $border-radius, and $font-family are defined in a separate SCSS variables file. This is important for maintaining consistency across your styles and making updates easier.
src/styles/comp.scss
Outdated
| &__star { | ||
| width: 16px; | ||
| height: 16px; | ||
| background-image: url(../images/star.svg); | ||
| background-repeat: no-repeat; | ||
| background-position: center; | ||
| margin-right: 4px; | ||
| } |
There was a problem hiding this comment.
The class comp__rate--stars__star is not following BEM conventions correctly. The double underscore is used for elements, and it should be comp__rate__star if it's an element of comp__rate, or comp__rate--star if it's a modifier.
src/styles/comp.scss
Outdated
| &--stars--4 .comp__rate--stars__star:nth-child(-n + 4) { | ||
| background-image: url(../images/star-active.svg); | ||
| } |
There was a problem hiding this comment.
The class comp__rate--stars--4 is not following BEM naming conventions correctly. In BEM, modifiers should be separated by a single hyphen, not double. Consider renaming it to something like comp__rate--stars-4.
src/styles/comp.scss
Outdated
| &--1 { | ||
| color: $text-color; | ||
| font-weight: 400; | ||
| font-size: 12px; | ||
| } | ||
|
|
||
| &--2 { | ||
| color: $text-color-strong; | ||
| font-weight: 700; | ||
| font-size: 16px; | ||
| } |
There was a problem hiding this comment.
The classes comp__price--1 and comp__price--2 do not clearly convey their purpose. In BEM, modifiers should describe the state or style of the block or element. Consider using more descriptive names, such as comp__price--label and comp__price--value.
src/styles/comp.scss
Outdated
| margin-inline: $margin-inline; | ||
| font-family: $font-family; |
There was a problem hiding this comment.
Ensure that the variables $margin-inline, $font-family, $text-color-strong, and $text-color are defined in a separate SCSS variables file. This is important for maintaining consistency across your styles and making updates easier.
