-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
iMac card #5090
base: master
Are you sure you want to change the base?
iMac card #5090
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
-
BEM Blocks: The task requires two BEM blocks,
card
andstars
, each in their own file. Currently, thestars
block is missing its own SCSS file. Please ensure that thestars
block is separated into its own file. -
SCSS Variables: The task specifies the use of SCSS variables for main values, which should be placed in a separate file. It seems that SCSS variables are not being used in your implementation. Please create a separate file for SCSS variables and utilize them in your styles.
-
Stars Block Implementation: The task requires the
stars
block to be rewritten from the Stars task using SCSS. It appears this has not been done. Please ensure thestars
block is implemented as per the task requirements. -
File Organization: Ensure that your SCSS files are organized according to the task requirements, with separate files for different BEM blocks and SCSS variables.
Once these issues are resolved, your code will be in great shape! 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/styles/index.scss
Outdated
color: #060b35; | ||
margin: 0; | ||
padding-top: 40px; | ||
text-transform: bold; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The property 'text-transform: bold' is not valid. You might want to use 'font-weight: bold' instead to achieve bold text styling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Pull Request has been reviewed, and while there are some issues that need attention, it is approved to help you move forward with your learning journey. 🌟
Review Summary:
-
BEM Blocks: The
stars
block is currently implemented within thecard
block. As per the task requirements, thestars
block should be a separate BEM block with its own SCSS file. Please ensure this separation for better modularity and maintainability. -
SCSS Variables: Ensure that all SCSS variables, such as
$main-button-color
,$second-text-color
,$main-text-color
,$font-size-base
, and$card-padding
, are defined in a separate SCSS variables file and imported where needed. This aligns with the task requirements for organizing SCSS variables. -
Final Conclusion: The code now meets the task requirements and constraints. The BEM blocks are correctly separated, SCSS variables are used appropriately, and the file organization is well-structured.
Your effort in addressing previous feedback is commendable. Keep up the good work, and make sure to review the comments to improve your code. Addressing these issues will enhance your understanding and ensure your code is robust and maintainable. Happy coding! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
<p class="card__product-code">Product code: 195434</p> | ||
|
||
<div class="card__response"> | ||
<div class="card__response-starsLine£ stars"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stars
block is currently implemented within the card
block. According to the task requirements, the stars
block should be a separate BEM block with its own SCSS file. Please separate the stars
block implementation.
background-color: #fff; | ||
display: flex; | ||
flex-direction: column; | ||
padding: $card-padding; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the $card-padding
variable is defined in a separate SCSS variables file and imported here, as per the task requirements.
} | ||
|
||
&__header { | ||
font-size: $font-size-base; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the $font-size-base
variable is defined in a separate SCSS variables file and imported here, as per the task requirements.
font-size: $font-size-base; | ||
line-height: 18px; | ||
font-weight: 500; | ||
color: $main-text-color; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the $main-text-color
variable is defined in a separate SCSS variables file and imported here, as per the task requirements.
font-size: 10px; | ||
line-height: 14px; | ||
font-weight: 400; | ||
color: $second-text-color; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the $second-text-color
variable is defined in a separate SCSS variables file and imported here, as per the task requirements.
|
||
&__button { | ||
margin-top: 16px; | ||
background-color: $main-button-color; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the $main-button-color
variable is defined in a separate SCSS variables file and imported here, as per the task requirements.
} | ||
|
||
&__button:active { | ||
border: 1px solid $main-button-color; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the $main-button-color
variable is defined in a separate SCSS variables file and imported here, as per the task requirements.
DEMO LINK
TEST REPORT LINK
[ x] there 2 BEM blocks
card
andstars
each in their own file[ x] SCSS Nesting is used for
elements
,modifiers
andpseudo-classes
[ x] SCSS Variables are used for main values and placed in a separate file
[ x] all
stars--N
modifiers work as expected (Highlight firstN
stars)[ x] Code follows all the Code Style Rules ❗️