loyout_product_card_reviev #4816

nikalaiii · 2024-10-05T14:38:40Z

mateacademy-ai-mentor

Hey there! 👋 You've made a great effort, but we need to tidy up a few things before we can merge this Pull Request. There are some issues with duplicate variable declarations, incorrect usage of selectors, and redundant properties. Also, make sure to set the base font size correctly and optimize the stars implementation for better efficiency. Keep up the good work, and with a bit of polishing, your code will shine! ✨

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

mateacademy-ai-mentor · 2024-10-05T14:39:01Z

src/index.html

+    <link
+      href="https://fonts.googleapis.com/css2?family=Roboto:wght@100;400;500;700&display=swap"
+      rel="stylesheet"
+    />
    <title>Product cards</title>
    <link
      rel="stylesheet"
      href="./styles/index.scss"


Link to SCSS file is incorrect. SCSS files are not supported by browsers directly. You should link to the compiled CSS file instead.

mateacademy-ai-mentor · 2024-10-05T14:39:01Z

src/index.html

+          <div class="card__rating-stars card__rating-stars--1"></div>
+          <div class="card__rating-stars card__rating-stars--2"></div>
+          <div class="card__rating-stars card__rating-stars--3"></div>
+          <div class="card__rating-stars card__rating-stars--4"></div>
+          <div class="card__rating-stars card__rating-stars--5"></div>


The way you've implemented stars for rating is not efficient. Consider using a loop in your SCSS to generate the stars, or use a single container with a background image that represents the stars, and control the filled portion with width and background clipping.

mateacademy-ai-mentor · 2024-10-05T14:39:01Z

src/index.html

+          <a
+            href="index.html"
+            class="card__buy-text"
+            data-qa="hover"
+          >


Anchor tag with 'href' attribute set to 'index.html' will lead to the same page. If this is a placeholder for a future link, consider using '#' or 'javascript:void(0);' for now to make it clear that it's not meant to be a navigable link yet.

mateacademy-ai-mentor · 2024-10-05T14:39:01Z

src/styles/index.scss

+html {
+  font-family: $font-family;
+}


According to the Code Style Rules, the base font size should be set in the html selector using px units. Make sure that the $font-family variable is defined with a fallback system font, e.g., Helvetica, Arial, sans-serif.

mateacademy-ai-mentor · 2024-10-05T14:39:01Z

src/styles/index.scss

+  width: 198px;
+  height: 406px;
+
+  &__content {


Avoid setting the same width and height for an element and its BEM element. If .card has a set width and height, the &__content should not repeat those properties unless it is intentionally different.

mateacademy-ai-mentor · 2024-10-05T14:39:01Z

src/styles/index.scss

+    background-repeat: no-repeat;
+    margin-left: 19px;
+    margin-top: 32px;
+  }


The background-position, background-size, and background-repeat properties are redundant here since they are already included in the flex-center mixin. It's better to keep the code DRY (Don't Repeat Yourself) by not repeating these properties.

mateacademy-ai-mentor · 2024-10-05T14:39:01Z

src/styles/index.scss

+    margin-top: 16px;
+
+    // Цикл для зірок
+    @for $i from 1 through $stars-count {


The loop for generating stars should be applied to a list of elements rather than applying styles to the &-stars class. Ensure that the HTML has multiple elements with the class &__rating-star to apply this loop correctly.

mateacademy-ai-mentor · 2024-10-05T14:39:01Z

src/styles/index.scss

+    @for $i from 1 through $stars-count {
+      &-stars:nth-child(#{$i}) {
+        // Перевіряємо, чи це остання зірка
+        @if $i < $stars-count {
+          background-image: url(../images/star-active.svg);
+        } @else {
+          background-image: url(../images/star.svg);
+        }
+
+        @include star-position;


The nth-child selector is being used incorrectly. It should be applied to child elements of a parent container and not to the &-stars modifier itself. Make sure that the HTML structure matches this SCSS logic.

mateacademy-ai-mentor · 2024-10-05T14:39:02Z

src/styles/variables.scss

+$font-family: 'Roboto', sans-serif;
+$border-color: white;
+$border-radius: 5px;
+$star-size: 16px;
+$buy-button-font-size: 14px;
+$buy-button-font-weight: 700;
+$stars-count: 5;


There are duplicate variable declarations in this file. The variables $font-family, $border-color, $border-radius, $star-size, $buy-button-font-size, $buy-button-font-weight, and $stars-count are defined twice with the same values. According to the Code Style Rules ❗️, you should avoid code duplication. Please remove the duplicate variable declarations.

mateacademy-ai-mentor

Great job on your project! The HTML structure is well-formed, and your SCSS/CSS is well organized. Just remember to validate your HTML and make sure to manage your dimensions and colors through variables for better maintainability. Keep up the good work! 🌟

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

mateacademy-ai-mentor · 2024-10-05T16:02:28Z

src/index.html

 <!doctype html>
 <html lang="en">


The <!DOCTYPE html> declaration should be in lowercase to adhere to the HTML5 specification and maintain consistency with the code style guide.

mateacademy-ai-mentor · 2024-10-05T16:02:28Z

src/index.html

+    />
+    <link
+      rel="preconnect"
+      href="https://fonts.gstatic.com"


The preconnect to fonts.gstatic.com is unnecessary if you're using fonts.googleapis.com, as the latter should implicitly handle the connection to the font service.

mateacademy-ai-mentor · 2024-10-05T16:02:28Z

src/index.html

    <title>Product cards</title>
    <link
      rel="stylesheet"
-      href="./styles/index.scss"
+      href="./styles/styles.css"


According to the checklist, the project should not contain unused files. If 'styles.css' is a compiled file from SCSS, it should not be included directly in the HTML, but rather served through a build process to ensure the latest styles are applied.

mateacademy-ai-mentor · 2024-10-05T16:02:28Z

src/index.html

+      class="card"
+      data-qa="card"
+    >
+      <div class="card__content">


The div element with class card__content could be replaced with a more semantic element such as article or section to better represent the content within a card.

mateacademy-ai-mentor · 2024-10-05T16:02:28Z

src/index.html

+        <div class="card__image"></div>
+
+        <div class="card__title">
+          <p class="card__title-text">


The p element inside card__title is unnecessary if it only contains text. You can apply the card__title-text class directly to the div element to reduce HTML complexity.

mateacademy-ai-mentor · 2024-10-05T16:02:29Z

src/styles/styles.css

+.card__content {
+  width: 198px;
+  height: 406px;
+  border-radius: 5px;
+}


The .card__content class has the same fixed width and height as .card. If .card__content is meant to be the same size as .card, it's better to use inheritance or relative units instead of redeclaring the same dimensions.

mateacademy-ai-mentor · 2024-10-05T16:02:29Z

src/styles/styles.css

+  margin-left: 19px;
+  margin-top: 32px;


The margin-left and margin-top for .card__image can be replaced with more flexible solutions like flexbox alignment or padding on the parent element.

mateacademy-ai-mentor · 2024-10-05T16:02:29Z

src/styles/styles.css

+.card__rating-stars:nth-child(1) {
+  background-image: url(../images/star-active.svg);
+  width: 16px;
+  height: 16px;
+  margin-left: 4px;
+  background-position: center;
+  background-repeat: no-repeat;
+}


The styles for .card__rating-stars are repeated with only the background-image changing for the last child. Consider using a general class for shared styles and a modifier class to change the background image for the inactive star.

mateacademy-ai-mentor · 2024-10-05T16:02:29Z

src/styles/styles.css

+.card__price {
+  display: flex;
+  justify-content: center;
+  align-items: center;
+  width: 166px;
+  height: 16px;
+  margin-top: 24px;
+  margin-left: 16px;
+}


The .card__price has a fixed width which may not be flexible. Consider using max-width or percentages for width to maintain flexibility.

mateacademy-ai-mentor · 2024-10-05T16:02:29Z

src/styles/styles.css

+.card__buy {
+  display: flex;
+  justify-content: center;
+  align-items: center;
+  width: 166px;
+  height: 40px;
+  margin-top: 16px;
+  margin-left: 16px;
+  background-color: #00acdc;
+  border: 1px solid #00acdc;
+  border-radius: 5px;
+}


The .card__buy button has fixed dimensions which can lead to content overflow if the text is too long. Use padding instead of fixed height for better text accommodation and consider min-width for better responsiveness.

nikalaiii added 8 commits October 5, 2024 13:40

maybe finish code

5ad8838

readme

9418e6a

commit

cdc6315

commit2

b706281

commit3

f3f904f

commit4

f4fe883

commit4

92641b6

commit

f3ba642

mateacademy-ai-mentor suggested changes Oct 5, 2024

View reviewed changes

nikalaiii added 2 commits October 5, 2024 17:50

maybe finish

3e8175c

com

bba615e

nikalaiii requested a review from mateacademy-ai-mentor October 5, 2024 16:01

mateacademy-ai-mentor approved these changes Oct 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

loyout_product_card_reviev #4816

loyout_product_card_reviev #4816

nikalaiii commented Oct 5, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Oct 5, 2024

mateacademy-ai-mentor Oct 5, 2024

mateacademy-ai-mentor Oct 5, 2024

mateacademy-ai-mentor Oct 5, 2024

mateacademy-ai-mentor Oct 5, 2024

mateacademy-ai-mentor Oct 5, 2024

mateacademy-ai-mentor Oct 5, 2024

mateacademy-ai-mentor Oct 5, 2024

mateacademy-ai-mentor Oct 5, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Oct 5, 2024

mateacademy-ai-mentor Oct 5, 2024

mateacademy-ai-mentor Oct 5, 2024

mateacademy-ai-mentor Oct 5, 2024

mateacademy-ai-mentor Oct 5, 2024

mateacademy-ai-mentor Oct 5, 2024

mateacademy-ai-mentor Oct 5, 2024

mateacademy-ai-mentor Oct 5, 2024

mateacademy-ai-mentor Oct 5, 2024

mateacademy-ai-mentor Oct 5, 2024

loyout_product_card_reviev #4816

Are you sure you want to change the base?

loyout_product_card_reviev #4816

Conversation

nikalaiii commented Oct 5, 2024

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment