final-code #5067
final-code #5067
Conversation
pryge
commented
Dec 15, 2024
pryge
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Thank you for your hard work on this project! 😊 While you've made great progress, there are a few critical issues that need to be addressed to meet the core requirements of the task:
-
BEM Blocks Separation: The task requires separate files for the
cardandstarsBEM blocks. Currently, both are in theindex.scssfile. Please separate them into their own files. -
SCSS Variables: The task specifies using SCSS variables for main values in a separate file. Ensure that you implement this requirement.
-
SCSS Nesting: While there is some nesting, please verify that all required nesting for elements, modifiers, and pseudo-classes is correctly implemented.
-
Stars Block: Ensure that the
starsblock is rewritten from the Stars task using SCSS as required. -
Stylesheet Link: The HTML file links to a
.scssfile, which needs to be preprocessed into CSS. Make sure the correct CSS file is linked.
Addressing these issues will help ensure your code meets the task requirements. Keep up the great work, 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.
src/index.html
Outdated
| @@ -11,8 +11,55 @@ | |||
| rel="stylesheet" | |||
| href="./styles/index.scss" | |||
There was a problem hiding this comment.
The href attribute is linking to a '.scss' file. Ensure that this file is being preprocessed into a CSS file, as browsers cannot directly interpret SASS files. If this is intended to be a CSS file, make sure the correct file is linked.
| body { | ||
| margin: 0; | ||
| font-family: Roboto, serif; |
There was a problem hiding this comment.
The font-family declaration includes 'Roboto' with a fallback of 'serif'. Since 'Roboto' is a sans-serif font, it would be more appropriate to use a sans-serif fallback, such as 'sans-serif'.
| } | ||
|
|
||
| .button { | ||
| font-family: Roboto, serif; |
There was a problem hiding this comment.
Similar to the body, the font-family declaration for the button includes 'Roboto' with a fallback of 'serif'. Consider using 'sans-serif' as the fallback to match the style of 'Roboto'.
There was a problem hiding this comment.
Great job on making progress with your project! 🎉 While there are some areas that need attention, such as ensuring SCSS variables are in a separate file and adhering to the BEM naming convention, your efforts in separating BEM blocks and implementing SCSS nesting are commendable. Please review the comments to improve your code, especially regarding the stylesheet link and class naming. Addressing these issues will enhance your code's maintainability and functionality. 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! ✨
| <link | ||
| rel="stylesheet" | ||
| href="./styles/index.scss" | ||
| href="../src/styles/index.css" |
There was a problem hiding this comment.
The href attribute is currently pointing to a .scss file. Make sure to link to the corresponding .css file that is generated after preprocessing the SCSS.
| height: 408px; | ||
| border: 1px solid #f3f3f3; | ||
| border-radius: 5px; | ||
| background-color: #fff; |
There was a problem hiding this comment.
Consider using SCSS variables for colors and other main values instead of hardcoding them. This will make it easier to manage and update styles.
| padding: 32px 16px 16px; | ||
| } | ||
|
|
||
| .imac-img { |
There was a problem hiding this comment.
The class .imac-img does not follow the BEM naming convention. Consider renaming it to something like .card__image to maintain consistency.
| display: block; | ||
| } | ||
|
|
||
| .name { |
There was a problem hiding this comment.
The class .name does not follow the BEM naming convention. Consider renaming it to something like .card__name to maintain consistency.
| margin-bottom: 4px; | ||
| } | ||
|
|
||
| .product-code { |
There was a problem hiding this comment.
The class .product-code does not follow the BEM naming convention. Consider renaming it to something like .card__code to maintain consistency.
| color: #616070; | ||
| } | ||
|
|
||
| .stars-review { |
There was a problem hiding this comment.
The class .stars-review does not follow the BEM naming convention. Consider renaming it to something like .card__stars-review to maintain consistency.
| margin-top: 16px; | ||
| } | ||
|
|
||
| .card-review { |
There was a problem hiding this comment.
The class .card-review does not follow the BEM naming convention. Consider renaming it to something like .card__reviews to maintain consistency.
| } | ||
| } | ||
|
|
||
| .price { |
There was a problem hiding this comment.
The class .price does not follow the BEM naming convention. Consider renaming it to something like .card__price to maintain consistency.
| } | ||
| } | ||
|
|
||
| .button { |
There was a problem hiding this comment.
The class .button does not follow the BEM naming convention. Consider renaming it to something like .card__button to maintain consistency.
| } | ||
| } | ||
|
|
||
| .card__stars { |
There was a problem hiding this comment.
The .card__stars class should ideally be part of the card block styles, not the stars block. Consider moving this style to the appropriate SCSS file for the card block to maintain separation of concerns.
