Develop #4815
Develop #4815
Conversation
There was a problem hiding this comment.
Hey there! 👋 It looks like there are a few important areas that need some attention before we can approve your Pull Request. Specifically, there's a misnamed BEM modifier that doesn't match its functionality, some missing file extensions in import statements, and a few other issues like typos and missing image references. These are crucial for maintaining the integrity and readability of the code. Don't worry, with some quick fixes, you'll be on the right track! Remember, every coder starts somewhere, and every mistake is an opportunity to learn and grow. Keep up the good work! 💪
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <link | ||
| rel="preconnect" | ||
| href="https://fonts.googleapis.com" | ||
| /> | ||
| <link | ||
| rel="preconnect" | ||
| href="https://fonts.gstatic.com" | ||
| /> |
There was a problem hiding this comment.
According to the Code Style Rules ❗️, you should combine the preconnect link tags into one. You can specify multiple href attributes separated by a comma.
| <h3 class="card__title"> | ||
| APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
| </h3> | ||
| <p class="card__sudtitle">Product code: 195434</p> |
There was a problem hiding this comment.
There is a typo in the class name card__sudtitle. It should be card__subtitle to follow proper English spelling and to be consistent with BEM naming conventions.
| class="images" | ||
| ></a> |
There was a problem hiding this comment.
Empty links are not allowed. If this is supposed to be an image container, you should include an img tag inside the a tag with the appropriate src and alt attributes.
| <link | ||
| href="https://fonts.googleapis.com/css2?family=Roboto:ital,wght@0,100;0,200;0,300;0,400;0,500;0,600;0,700;0,800;0,900;1,100;1,200;1,300;1,400;1,500;1,600;1,700;1,800;1,900&display=swap" | ||
| rel="stylesheet" | ||
| /> | ||
| <link | ||
| rel="stylesheet" | ||
| href="./styles/index.scss" |
There was a problem hiding this comment.
The href attribute should point to a .css file, not a .scss file. Ensure that the SCSS files are being compiled to CSS and that the compiled CSS file is correctly linked in the href attribute.
| &__sudtitle { | ||
| margin: 4px 0 0; | ||
| padding-top: 0; | ||
| font-weight: 400; | ||
| line-height: 14px; | ||
| color: #616070; | ||
| font-size: 10px; | ||
| } |
There was a problem hiding this comment.
It appears there is a typo in the BEM element name &__sudtitle. It should be &__subtitle to follow the proper naming convention and maintain consistency.
| &__buy-button { | ||
| margin: 16px 0 0; | ||
| background-color: #00acdc; | ||
| position: relative; | ||
| width: 166px; | ||
| height: 40px; | ||
| border-radius: 5px; | ||
| color: #fff; | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| text-align: center; | ||
| font-weight: 700; | ||
| font-size: 14px; | ||
| line-height: 16px; | ||
| text-decoration: none; | ||
| box-sizing: border-box; | ||
| &:hover { | ||
| background: #fff; | ||
| color: #00acdc; | ||
| border: 1px solid #00acdc; |
There was a problem hiding this comment.
Ensure that the hover state for the &__buy-button is consistent with the design requirements. If the design specifies different colors or styles, they should be implemented accordingly. Additionally, when changing background-color on hover, it's better to transition the color change for a smoother visual effect. Consider adding transition: background-color 0.3s; to the &__buy-button block.
| .images { | ||
| background-image: url(../images/imac.jpeg); | ||
| background-repeat: no-repeat; | ||
| background-size: cover; | ||
| display: block; | ||
| width: 160px; | ||
| height: 134px; | ||
| margin: 0 3px; | ||
| } |
There was a problem hiding this comment.
The class name .images suggests that it can be applied to multiple elements, but the styles within are very specific to a single image. Consider renaming the class to more accurately reflect its purpose, such as .product-image or .card-image, to improve readability and maintainability of the code.
| @@ -0,0 +1,9 @@ | |||
| .images { | |||
| background-image: url(../images/imac.jpeg); | |||
There was a problem hiding this comment.
According to the Code Style Rules ❗️, for SCSS files, it is recommended to use variables for colors, fonts, and other static values. Consider storing image paths as variables at the top of your SCSS files for easier maintenance and reusability.
| body { | ||
| @import '../styles/card'; | ||
| @import '../styles/image'; | ||
| @import '../styles/starts'; |
There was a problem hiding this comment.
According to the Code Style Rules, when importing several files, each import statement should be on a new line and should not have a file extension. The import statements here are correctly placed on separate lines, but the file extensions are missing. Please add '.scss' to the end of each import path.
| &--0 { | ||
| .stars__star:nth-child(-n + 4) { | ||
| background-image: url(../images/star-active.svg); /* Active star image */ | ||
| } |
There was a problem hiding this comment.
The BEM modifier stars--0 seems to be misnamed. According to the checklist, modifiers should represent different states or versions of a block or an element. However, stars--0 implies it is for zero stars, which doesn't make sense as this code is setting the star-active image for the first four stars. Ensure that the modifier name and the associated style rules accurately represent the intended design and functionality.
| background-repeat: no-repeat; | ||
| background-size: cover; | ||
| display: block; | ||
| width: 160px; |
DEMO LINK
Я маю проблеми з деплоєм і ментор сказав пушити так
https://ibb.co/3vXxmX4(посилання на фото тестів)