Develop #5262
Develop #5262
Conversation
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 before we can approve the pull request. Specifically, the task requires separate files for the BEM blocks card and stars, but the stars block file is missing. Additionally, the stars--N modifiers are not implemented, which is a core requirement. Please make these changes, and you'll be all set! 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.
src/styles/main.scss
Outdated
| @@ -0,0 +1,148 @@ | |||
| @import '/src/styles/variables'; | |||
There was a problem hiding this comment.
The import path for variables should be relative to the current file. Consider changing '/src/styles/variables' to './variables' to ensure the path is correct.
src/styles/main.scss
Outdated
|
|
||
| &__starlist .main__starlist:hover { | ||
| display: ''; | ||
| background-image: url(/src/images/star-active.svg); | ||
| } |
There was a problem hiding this comment.
The background-image URL should be relative to the index.scss file as per the task requirements. Change the path to ../images/star-active.svg.
src/styles/main.scss
Outdated
| background-image: url(/src/images/star.svg); | ||
| background-size: cover; | ||
| background-repeat: no-repeat; |
There was a problem hiding this comment.
The background-image URL should be relative to the index.scss file as per the task requirements. Change the path to ../images/star.svg.
src/styles/main.scss
Outdated
| &__starlist .main__star:nth-child(-n + 4) { | ||
| background-image: url(/src/images/star-active.svg); | ||
| width: 13px; | ||
| height: 12px; | ||
| } |
There was a problem hiding this comment.
The background-image URL should be relative to the index.scss file as per the task requirements. Change the path to ../images/star-active.svg.
src/styles/variables.scss
Outdated
| $mainAccent-color: #060b35; | ||
| $mainSecondary-color: #616070; | ||
| $mainBlueAccent-color: #00acdc; | ||
| $mainAccent-color: #060b35; |
There was a problem hiding this comment.
There is a duplicate definition of $mainAccent-color. Remove the duplicate to avoid confusion and potential issues.
There was a problem hiding this comment.
Great job on your submission! 🎉 While your code is well-structured and follows good practices like BEM methodology and SCSS partials, there are a few areas for improvement. Specifically, the stars--N modifiers are missing, which are crucial for implementing different star ratings as per the task requirements. Additionally, ensure that the SCSS file is correctly compiled by the Parcel bundler and that all variables are defined to avoid compilation errors.
The decision to approve is made to help you move forward with your learning journey. Please review the comments and address the issues to enhance your code quality. Keep up the good work and continue to learn from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| &__starlist .main__star:nth-child(-n + 4) { | ||
| background-image: url(../images/star-active.svg); | ||
| width: 13px; | ||
| height: 12px; | ||
| } |
There was a problem hiding this comment.
The stars--N modifiers are missing. According to the task requirements, you need to implement these modifiers to represent different star ratings (e.g., stars--1, stars--2, etc.).
DEMO LINK
TEST REPORT LINK
there 2 BEM blocks
cardandstarseach in their own fileSCSS Nesting is used for
elements,modifiersandpseudo-classesSCSS Variables are used for main values and placed in a separate file
all
stars--Nmodifiers work as expected (Highlight firstNstars)Code follows all the Code Style Rules ❗️