-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 #4821
base: master
Are you sure you want to change the base?
add task solution #4821
Conversation
dmdamyan
commented
Oct 7, 2024
- DEMO LINK
- [TEST REPORT LINK](https://dmdamyan.github.io/layout_product-cards/report/
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.
Hey there! 👋 You're doing great, but we need to address a few important issues before moving forward. It's important to ensure that our code is both semantic and follows best practices. Let's correct the non-semantic use of images for text content, ensure proper use of HTML tags for better SEO, and fix any typos in the class names. Once these changes are made, we'll be on track for approval! Keep up the good work, and we'll get there together! 💪
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/index.html
Outdated
<body | ||
class="card" | ||
data-qa="card" | ||
> |
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 body
element should not have classes or attributes that are specific to a component. Instead, wrap the content inside a div
or section
with the card
class and the data-qa
attribute.
src/index.html
Outdated
class="card" | ||
data-qa="card" | ||
> | ||
<div class="card__image"></div> |
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.
It's a common practice to use img
tags for images. If card__image
is meant to display an image, consider using <img src="path-to-image" alt="description" class="card__image">
with appropriate src
and alt
attributes.
src/index.html
Outdated
> | ||
<div class="card__image"></div> | ||
|
||
<div class="card__model"> |
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 product model should be wrapped in a heading tag (like h1
, h2
, etc.) to define it as a title and improve semantic meaning and SEO.
src/index.html
Outdated
|
||
<div class="card__product-code">Product code: 195434</div> | ||
|
||
<div class="card__grade"> |
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.
For the .card__grade-stars
, consider using an ul
with li
elements for each star to improve semantics, as this is a list of items (stars).
src/index.html
Outdated
<div class="card__grade-reviews">Reviews: 5</div> | ||
</div> | ||
|
||
<div class="card__price"></div> |
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 .card__price
div is empty. If it's supposed to contain a price, it should include the price inside, and use a span
or p
tag with a class to style it.
display: flex; | ||
flex-direction: column; | ||
align-items: center; | ||
box-sizing: border-box; |
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.
It is better to set box-sizing: border-box;
globally to apply it to all elements. Consider adding *, *::before, *::after { box-sizing: border-box; }
at the beginning of your stylesheet.
src/styles/index.scss
Outdated
font-size: 12px; | ||
font-weight: 500; | ||
line-height: 18px; | ||
text-align: left; |
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.
According to the Code Style Rules, text should not be aligned to the left or right. It should be centered or justified. Please correct text-align: left;
to text-align: center;
if that's the design requirement.
src/styles/index.scss
Outdated
} | ||
|
||
&__price { | ||
background-image: url(../images/price.png); |
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.
Using images for elements like price seems to be a non-semantic approach. If it's a static component, consider using HTML and CSS to create the price's appearance instead of an image.
width: 166px; | ||
height: 40px; | ||
border-radius: 5px; | ||
border-color: inherit; |
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 border-color: inherit;
is unnecessary if you are not setting a specific border style or width. If you want to ensure the button has a border, specify it explicitly with border-style
and border-width
.
border-radius: 5px; | ||
border-color: inherit; | ||
background-color: #00acdc; | ||
margin-top: 16px; |
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 margin-top
property is used quite often with the same value of 16px
. Consider creating a utility class or a mixin to avoid repetition and make the code DRY (Don't Repeat Yourself).
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.
While you've made great progress, let's address the responsiveness and maintainability concerns related to the fixed widths and heights before finalizing the Pull Request. Your effort in following BEM conventions and using SCSS features is commendable! 👍 Just a few tweaks to ensure the code is flexible and future-proof, and you'll be all set! Remember, making these changes now will save you time and effort in the long run. Keep up the good work! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/index.html
Outdated
<div | ||
class="card" | ||
data-qa="card" | ||
> |
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.
Consider using a more semantic tag for the card container, such as <article>
or <section>
, as it represents a self-contained composition in a document, page, application, or site, which is intended to be independently distributable or reusable.
src/index.html
Outdated
<div class="card__model"> | ||
<h2 class="card__model-name"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h2> | ||
</div> |
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 <h2>
element should be directly within the card
block for better semantic structure and SEO. There's no need for an extra div
wrapper around it.
src/index.html
Outdated
<div class="card__product-code"> | ||
<h3 class="card__product-code-style">Product code: 195434</h3> | ||
</div> |
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 product code can be marked up as a paragraph (<p>
) instead of a heading (<h3>
), as it is not a title of a section. Semantic HTML elements should be used appropriately to improve accessibility and SEO.
<div class="card__grade"> | ||
<div class="card__grade-stars"> | ||
<ul class="card__grade-stars-list"> | ||
<li class="card__grade-stars-list-1"></li> | ||
|
||
<li class="card__grade-stars-list-2"></li> | ||
|
||
<li class="card__grade-stars-list-3"></li> | ||
|
||
<li class="card__grade-stars-list-4"></li> | ||
|
||
<li class="card__grade-stars-list-5"></li> | ||
</ul> | ||
</div> | ||
|
||
<div class="card__grade-reviews">Reviews: 5</div> | ||
</div> |
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 rating and reviews could be better represented using <figure>
and <figcaption>
elements to group the star ratings and the review count semantically.
src/index.html
Outdated
<p class="card__price-styleText">Price:</p> | ||
|
||
<p class="card__price-stylePrice">$2,199</p> |
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 class name card__price-stylePrice
is not following BEM naming conventions. It should be something like card__price-amount
to represent an element of the card__price
block.
src/styles/index.scss
Outdated
width: 160px; | ||
height: 134px; |
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.
Using fixed width and height for images can cause distortion or cropping if the image's aspect ratio doesn't match. Consider using properties like max-width, max-height, and object-fit to control the image size while preserving its aspect ratio.
width: 166px; | ||
height: 36px; |
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.
Fixed width and height on text containers can lead to text overflow or clipping issues. Use padding along with min-height or min-width as needed, and ensure text has enough room to scale with different screen sizes or font settings.
background-image: url(../images/star.png); | ||
width: 14px; | ||
height: 14px; | ||
} |
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 use of display: block;
on a li
element (which is block-level by default) is unnecessary. If you need to apply specific styles to list items, consider targeting them with a class instead.
src/styles/index.scss
Outdated
display: none; | ||
} |
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.
Using display: none;
to hide an element is not recommended if the element might be shown later. If this is for a dynamic feature like a rating system, consider toggling visibility with a class instead.
src/styles/index.scss
Outdated
&-stylePrice { | ||
margin: 0; | ||
width: 50px; | ||
height: 18px; | ||
font-size: 16px; | ||
font-weight: 700; | ||
line-height: 18px; | ||
color: #060b35; |
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.
Be cautious with fixed widths on elements containing text. The text might overflow or wrap in an undesired way on different screens or with different font sizes. Consider using min-width or simply 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.
Great effort on the code! 👍 Remember to specify border colors explicitly when needed, use display: none
for elements that shouldn't take up space, and apply BEM naming conventions consistently. Also, consider simplifying redundant selectors and ensuring attributes like crossorigin
are only used when necessary. Keep up the good work and use these tips to enhance your code in the future! 💡
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
<link | ||
rel="preconnect" | ||
href="https://fonts.gstatic.com" | ||
crossorigin="anonymous" |
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.
According to the Code Style Rules, attribute crossorigin
should be used with the link
tag only when it is necessary. In this case, it seems redundant as the Google Fonts API does not require CORS for font serving.
class="card" | ||
data-qa="card" | ||
> | ||
<div class="card__image"> |
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 BEM naming convention suggests that elements of a block should be named using __
to separate the block from the element. Here, card__image
is correct, but the nested img
element should follow the same convention and be named something like card__photo
instead of card__image-photo
.
</h2> | ||
|
||
<div class="card__product-code"> | ||
<p class="card__product-code-style">Product code: 195434</p> |
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 p
tag with class card__product-code-style
should be named according to BEM methodology. Since it is an element of the card
block, it should be named card__product-code
.
<div class="card__grade"> | ||
<div class="card__grade-stars"> | ||
<ul class="card__grade-stars-list"> | ||
<li class="card__grade-stars-list-1"></li> |
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 BEM naming convention suggests that elements should not be named with numbers at the end. Instead, use a modifier if necessary. For example, card__star
and card__star--1
, card__star--2
, etc., if different styles are needed for each star.
</div> | ||
|
||
<div class="card__price"> | ||
<p class="card__price-style">Price:</p> |
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.
Similar to the previous comment, the p
tag with class card__price-style
should follow the BEM naming convention. It could be named card__price-label
to indicate its purpose more clearly.
<div class="card__price"> | ||
<p class="card__price-style">Price:</p> | ||
|
||
<p class="card__price-amount-style">$2,199</p> |
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 p
tag with class card__price-amount-style
should follow the BEM naming convention. It could be named card__price-amount
to be consistent with BEM.
&__image { | ||
margin-top: 33px; | ||
|
||
&-photo { | ||
max-width: 160px; | ||
max-height: 134px; | ||
object-fit: contain; | ||
} |
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.
According to the BEM methodology, an element should not be nested inside another element. Here, &__image-photo
is an element nested inside &__image
, which is also an element. Consider refactoring to follow BEM conventions, perhaps by using a modifier or a block instead.
justify-content: space-between; | ||
margin: 1px; | ||
|
||
&:nth-child(n + 1) { |
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 &:nth-child(n + 1)
selector seems redundant here since it applies to all child elements, effectively having no specific selection criteria. You can remove this selector and apply the styles directly to &-list
if there are no other children that the styles should not apply to.
&-5 { | ||
visibility: hidden; | ||
} |
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.
Setting an element to visibility: hidden
will hide it but still take up the same space as if it were visible. If the intent is to not display the fifth star at all, consider using display: none
instead. However, if the space needs to be reserved (but the star not shown), visibility: hidden
is correct.
width: 166px; | ||
height: 40px; | ||
border-radius: 5px; | ||
border-color: inherit; |
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 border-color: inherit;
declaration will inherit the border color from the parent element, but since there's no border color set on the parent, this line has no effect. If you want the button to have a border color, you should specify it explicitly.