Skip to content
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

add task solution #4820

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

OlhaLishenko
Copy link

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there! 👋 You've done a great job with the implementation, but there are a few critical issues that we need to address before merging. The duplicate border declarations and the unquoted URL in the CSS can potentially cause functionality problems, and the misuse of BEM naming conventions might lead to confusion in maintaining the code. Let's make these adjustments to ensure everything is in tip-top shape! Remember, clean and clear code is just as important as working code. Keep up the good work, and I'm sure you'll have it perfect in no time! 💪

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

src/index.html Outdated
@@ -11,8 +11,59 @@
rel="stylesheet"
href="./styles/index.scss"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the Code Style Rules, SCSS files should be linked in HTML as CSS files after they have been compiled. You should include the compiled CSS file instead of the SCSS file directly.

src/index.html Outdated
Comment on lines 28 to 32
<div
class="product-card"
id="product-card"
data-qa="card"
>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of both class and id with the same name product-card is redundant and can lead to confusion. It's better to use either a class or an id for styling and JavaScript hooks, but not both with the same name. If id is not used for JavaScript, you can remove it.

src/index.html Outdated
Comment on lines 35 to 40
<div class="product-content">
<div class="product-content__name">
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</div>

<div class="product-content__code">Product code: 195434</div>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class naming convention seems to be BEM, but product-content is not consistent with the parent block product-card. It should be product-card__content to maintain BEM conventions.

src/index.html Outdated
</div>

<div class="product-content__price">
<div class="text-price text-price--text">Price:</div>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class text-price--text does not follow the BEM methodology correctly. Modifier classes should not be used on their own; they should modify an existing block or element class. Consider renaming to product-content__price-text or similar.

src/index.html Outdated

<div class="product-content__price">
<div class="text-price text-price--text">Price:</div>
<div class="text-price text-price--value">$2,199</div>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class text-price--value is used incorrectly as a modifier without the base class. It should be used along with the base class, or you should rename it to follow the BEM convention, such as product-content__price-value.


.product-content {
margin: 40px 16px 16px;
font-family: $main-fonts, sans-serif;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have declared the font-family for '.product-content' and its elements multiple times (lines 36, 39, 47, 90, 98). You should define it once globally for the body or a common parent class unless they differ.

Comment on lines 24 to 26
background-image: url(../images/imac.jpeg);
background-repeat: no-repeat;
background-size: 100% 100%;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The background image URL should be quoted (url("../images/imac.jpeg")) to avoid potential issues with special characters or spaces in the file path.

line-height: 16px;
margin: 16px 0 0;
border-radius: $border-radius;
border: none;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The border property is set twice consecutively (lines 127 and 128). The second declaration overrides the first one, making the first declaration redundant.

Comment on lines 127 to 128
border: none;
border: 1px solid $button-color;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The border property is declared twice with the same value. Remove the duplicate declaration to clean up the code.

color: $button-color;
background-color: #ffff;
border: 1px solid $button-color;
box-sizing: border-box;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The box-sizing: border-box; declaration in the :hover state of .buy-button is redundant since it's already defined in the normal state (line 115).

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job on following the code style rules! There are a few recommendations for improvements, but none of them are critical issues that would significantly impact the functionality or security of your code. Keep up the good work, and consider these suggestions to further refine your CSS for future projects! 😊👍 Remember, attention to detail can really enhance the maintainability and scalability of your stylesheets.

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

src/index.html Outdated
Comment on lines 35 to 36
<div class="product-name">
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the checklist, text content should be wrapped in a paragraph <p> tag, not a <div>. Consider changing <div class="product-name"> to <p class="product-name">.

APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</div>

<div class="product-code">Product code: 195434</div>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the product code, it is more semantically correct to use a <span> tag since this is a short piece of information within a block. Change <div class="product-code"> to <span class="product-code">.

src/index.html Outdated
Comment on lines 54 to 55
<div class="product-price__text">Price:</div>
<div class="product-price__value">$2,199</div>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The price label and value seem to be a part of the same piece of information and should be within the same tag. Consider using a <span> tag for the price label and wrapping both the label and the value inside a <p> tag for better semantic structure.

src/index.html Outdated
Comment on lines 58 to 62
<a
href="#"
class="buy-button"
data-qa="hover"
>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anchor tags representing buttons should include a role="button" attribute to ensure accessibility. Add role="button" to the <a> tag.

Comment on lines +9 to +11
width: 200px;
border: 1px solid #f3f3f3;
border-radius: 5px;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the Code Style Rules, it's recommended to group CSS properties by their type. For instance, box model properties should be grouped together (width, border, border-radius, etc.), and positioning properties should be grouped together (display, align-items, justify-content, etc.). Consider reordering properties for better readability and maintainability.

Comment on lines 68 to 76
background-image: url('../images/star-active.svg');
width: 16px;
height: 16px;
background-repeat: no-repeat;
background-position: center;
}

.stars__star:last-of-type {
background-image: url('../images/star.svg');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using two different classes to apply a background image for active and inactive stars may not be the most efficient approach. Consider using a single class and modifying the background image with a modifier class or using a pseudo-class if the inactive state is based on user interaction.

background-size: 100% 100%;
width: 160px;
height: 134px;
margin: 32px 19px 0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'margin' shorthand is specified with pixel values, which is not very flexible. Consider using relative units like 'em' or 'rem' for margins to make the design more responsive and accessible.

justify-content: center;

&__image {
background-image: url('../images/imac.jpeg');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the checklist, images should be in the format of .png, .jpg, or .jpeg. The image 'imac.jpeg' should be checked to ensure it is in the correct format and renamed if necessary to adhere to the format guidelines.

Comment on lines +23 to +31
&__image {
background-image: url('../images/imac.jpeg');
background-repeat: no-repeat;
background-size: 100% 100%;
width: 160px;
height: 134px;
margin: 32px 19px 0;
box-sizing: border-box;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The BEM element '.product-card__image' has a margin with non-zero values, which is not recommended. BEM methodology suggests avoiding external geometry and positioning for BEM blocks and elements, such as margins. Consider refactoring this by utilizing the parent element to control the layout instead.

align-items: center;
background-color: $button-color;
text-decoration: none;
color: #ffff;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The color value should be written in lowercase to maintain consistency as per the code style rules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants