add task solution #5206
add task solution #5206
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! 😊 While your solution meets many of the task requirements, there are a few critical issues that need to be addressed before we can approve the pull request:
-
CSS Selectors: The CSS selectors for targeting stars within the rating system need adjustments. For example, selectors like
.stars--5:nth-child(-n+5)should be updated to.stars--5 .stars__star:nth-child(-n+5)to correctly apply styles to the stars within each div. -
HTML Element Selector: The CSS file incorrectly uses
.htmlas a selector. It should behtmlwithout the dot, ashtmlis an element, not a class. -
BEM Methodology: The task requires separate CSS files for each BEM block, but currently, all styles are in a single file. This needs to be corrected to adhere to the BEM methodology.
-
Semantic HTML: The use of
<a>tags for stars is semantically incorrect since they are not links. Consider using<div>or<span>tags instead.
Please make these changes to ensure the code fully complies with the task requirements. Keep up the great work, and don't hesitate to reach out if you need any help! 🌟
✨ 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/style.css
Outdated
| @@ -1 +1,43 @@ | |||
| /* add styles here */ | |||
| .html { | |||
There was a problem hiding this comment.
The selector .html should be html without the dot, as html is an HTML element, not a class.
src/style.css
Outdated
| width: 16px; | ||
| } | ||
|
|
||
| .stars--1:nth-child(-n+1) { |
There was a problem hiding this comment.
The selector .stars--1:nth-child(-n+1) is incorrect for targeting the first child. The .stars--1 class is applied to the parent div, not the individual stars. Consider using .stars--1 .stars__star:nth-child(-n+1) to target the first star within the .stars--1 div.
src/style.css
Outdated
| background-image: url(/src/images/star-active.svg); | ||
| } | ||
|
|
||
| .stars--2:nth-child(-n+2) { |
There was a problem hiding this comment.
Similar to the previous comment, the selector .stars--2:nth-child(-n+2) should be .stars--2 .stars__star:nth-child(-n+2) to correctly target the first two stars within the .stars--2 div.
src/style.css
Outdated
| background-image: url(/src/images/star-active.svg); | ||
| } | ||
|
|
||
| .stars--3:nth-child(-n+3) { |
There was a problem hiding this comment.
The selector .stars--3:nth-child(-n+3) should be .stars--3 .stars__star:nth-child(-n+3) to correctly target the first three stars within the .stars--3 div.
src/style.css
Outdated
| background-image: url(/src/images/star-active.svg); | ||
| } | ||
|
|
||
| .stars--4:nth-child(-n+4) { |
There was a problem hiding this comment.
The selector .stars--4:nth-child(-n+4) should be .stars--4 .stars__star:nth-child(-n+4) to correctly target the first four stars within the .stars--4 div.
src/style.css
Outdated
| background-image: url(/src/images/star-active.svg); | ||
| } | ||
|
|
||
| .stars--5:nth-child(-n+5) { |
There was a problem hiding this comment.
The selector .stars--5:nth-child(-n+5) should be .stars--5 .stars__star:nth-child(-n+5) to correctly target all five stars within the .stars--5 div.
There was a problem hiding this comment.
Great job on addressing the issues from the previous review! 🎉 Your code now adheres to the task requirements and constraints, and all the necessary corrections have been made. Here are some key points from the review:
- CSS Selectors: You've correctly updated the CSS selectors for targeting stars within the rating system. The format
.stars--N .stars__star:nth-child(-n+N)is now correctly applied. - HTML Element Selector: The previous error with the
.htmlselector has been resolved. - BEM Methodology: You've successfully separated the CSS files for each BEM block, with distinct files for
pageandstarsblocks. - Semantic HTML: The use of
<span>tags for stars is now semantically appropriate.
While the decision is to APPROVE this Pull Request to help you move forward with your learning, please ensure that you review the comments to further improve your code. Remember, addressing critical issues in the future will help you avoid similar problems and enhance your coding skills. Keep up the great work and continue learning from your experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| @@ -9,10 +9,56 @@ | |||
| <title>Stars</title> | |||
| <link | |||
| rel="stylesheet" | |||
| href="./style.css" | |||
| href="./styles/style.css" | |||
There was a problem hiding this comment.
Ensure that the linked CSS file style.css is correctly implementing the BEM methodology by having separate CSS files for each BEM block. This was a requirement mentioned in the previous review.
Typical MistakesfromBEMlesson theory are checked.