commit message #5064
commit message #5064
Conversation
Humenyuk1991
commented
Dec 14, 2024
Humenyuk1991
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Thank you for your hard work on this pull request! 😊 There are a few critical issues that need to be addressed before we can approve it. The most important ones include the incorrect nesting of div elements in the HTML, which could lead to rendering problems, and the missing separate SCSS file for the stars block, which is a core requirement. Additionally, ensure that SCSS variables are used and placed in a separate file as specified. Please address these issues to meet the task requirements. Keep up the great effort, and I'm confident you'll get it right! 🚀
✨ 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
| <link rel="preconnect" href="https://fonts.gstatic.com" | ||
| crossorigin="anonymous" | ||
| > |
There was a problem hiding this comment.
The crossorigin attribute should be part of the <link> tag on line 9. It should be written as <link rel="preconnect" href="https://fonts.gstatic.com" crossorigin="anonymous">.
| <img src="images/imac.jpeg" alt="Monitor"> | ||
| </div> | ||
| </div> | ||
|
|
There was a problem hiding this comment.
There is an extra closing div tag on line 26 that does not match any opening tag. This could cause rendering issues.
src/index.html
Outdated
| BUY | ||
| </button> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
There is an extra closing div tag on line 98 that does not match any opening tag. This could cause rendering issues.
src/styles/index.scss
Outdated
| .card__star:hover~.card__star svg { | ||
| background-color: #FFDE6A; | ||
| stroke-width: 0; | ||
| } |
There was a problem hiding this comment.
The background-color property is not applicable to SVG elements. You might want to use fill to change the color of the SVG on hover.
src/styles/index.scss
Outdated
| .card__reviews { | ||
| font-size: 10px; | ||
| font-weight: 400; | ||
| color: #fff; |
There was a problem hiding this comment.
The color for .card__reviews is set to white. Ensure that the background where this text appears is not white, or change the text color to ensure it is visible.
There was a problem hiding this comment.
Review Summary
Great job on addressing some of the previous issues! 🎉 Your efforts in improving the HTML structure and creating the stars.scss file are commendable. However, there are a few critical points that need attention to fully meet the task requirements:
-
SCSS Variables File: It's important to create a separate SCSS file for variables. This helps in maintaining consistency and makes it easier to manage main values across your stylesheets.
-
Nesting of SCSS: Ensure that all elements, modifiers, and pseudo-classes are properly nested according to the BEM methodology. This will enhance the readability and maintainability of your code.
-
Separate SCSS File for Stars Block: While the
stars.scssfile is correctly created, make sure all styles related to thestarsblock are encapsulated within this file and not duplicated elsewhere. -
HTML Structure: Your HTML structure is mostly correct, and the use of BEM methodology is consistent. Keep up the good work!
The decision to approve is made to help you move forward with your learning journey. Please review the comments and address these issues to improve your code. Remember, learning from these points will help you avoid similar problems in the future. Keep up the great work and continue to enhance your skills! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/index.html
Outdated
| <link | ||
| rel="stylesheet" | ||
| href="./styles/index.scss" | ||
| /> |
There was a problem hiding this comment.
Ensure that the SCSS file for the stars block is created and linked correctly. This is a core requirement as per the task description.
src/styles/index.scss
Outdated
| .stars { | ||
| display: flex; | ||
| gap: 4px; | ||
| } | ||
|
|
||
| .stars__star { | ||
| background-image: url(../images/star.svg); | ||
| background-repeat: no-repeat; | ||
| width: 16px; | ||
| height: 16px; | ||
| } | ||
|
|
||
| .stars--4 .stars__star:nth-child(-n + 4) { | ||
| background-image: url(../images/star-active.svg); | ||
| } |
There was a problem hiding this comment.
The SCSS for the stars block should be placed in a separate file as per the task requirements. Ensure that this block is moved to its own SCSS file.
src/styles/index.scss
Outdated
| border: 1px solid #f3f3f3; | ||
| border-radius: 5px; | ||
| width: 200px; | ||
| height: 406px; |
There was a problem hiding this comment.
Don't need to write fixed height for the card
| height: 406px; |
src/styles/index.scss
Outdated
| } | ||
|
|
||
| &__product { | ||
| width: 98px; |
There was a problem hiding this comment.
| width: 98px; |
src/styles/index.scss
Outdated
| } | ||
|
|
||
| &__button { | ||
| background-color: #00acdc; |
There was a problem hiding this comment.
Create a variable for repeated color and use it everywhere
