add solution #4869

Knysh19 · 2024-11-24T23:36:42Z

mateacademy-ai-mentor

Thank you for your hard work on this project! 😊 While your code meets many of the task requirements, there are a few critical issues that need to be addressed before we can approve the pull request. The scale transformation on hover is too large, which might affect the layout or cause overlap with other elements. Additionally, ensure that all CSS values include appropriate units, such as 'px', to maintain cross-browser compatibility. Lastly, consider using a templating approach or JavaScript to dynamically generate repeated product card elements to reduce redundancy and improve maintainability. Once these issues are resolved, your solution will be in great shape! Keep up the great work! 🚀

✨ 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.

mateacademy-ai-mentor · 2024-11-24T23:37:28Z

src/index.html

+        >
+          BUY
+        </a>
+      </div>
+
+      <div class="card">
+        <img
+          class="card__image"
+          src="./images/imac.jpeg"
+          alt="iMac"
+        />
+        <h2 class="card__title">
+          APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
+        </h2>
+        <p class="card__code">Product code: 195434</p>
+        <div class="card__review">
+          <div class="stars">
+            <div class="stars__star-active"></div>
+            <div class="stars__star-active"></div>
+            <div class="stars__star-active"></div>
+            <div class="stars__star-active"></div>
+            <div class="stars__star"></div>
+          </div>
+          <p class="card__review_count">Reviews: 5</p>
+        </div>
+        <div class="card__price">
+          <p class="card__price_text">Price:</p>
+          <p class="card__price_count">$2,199</p>
+        </div>
+        <a
+          href="#"
+          class="card__button"
+        >
+          BUY
+        </a>
+      </div>
+
+      <div class="card">
+        <img
+          class="card__image"
+          src="./images/imac.jpeg"
+          alt="iMac"
+        />
+        <h2 class="card__title">
+          APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
+        </h2>
+        <p class="card__code">Product code: 195434</p>
+        <div class="card__review">
+          <div class="stars">
+            <div class="stars__star-active"></div>
+            <div class="stars__star-active"></div>
+            <div class="stars__star-active"></div>
+            <div class="stars__star-active"></div>
+            <div class="stars__star"></div>
+          </div>
+          <p class="card__review_count">Reviews: 5</p>
+        </div>
+        <div class="card__price">
+          <p class="card__price_text">Price:</p>
+          <p class="card__price_count">$2,199</p>
+        </div>
+        <a
+          href="#"
+          class="card__button"
+        >
+          BUY
+        </a>
+      </div>
+
+      <div class="card">
+        <img
+          class="card__image"
+          src="./images/imac.jpeg"
+          alt="iMac"
+        />
+        <h2 class="card__title">
+          APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
+        </h2>
+        <p class="card__code">Product code: 195434</p>
+        <div class="card__review">
+          <div class="stars">
+            <div class="stars__star-active"></div>
+            <div class="stars__star-active"></div>
+            <div class="stars__star-active"></div>
+            <div class="stars__star-active"></div>
+            <div class="stars__star"></div>
+          </div>
+          <p class="card__review_count">Reviews: 5</p>
+        </div>
+        <div class="card__price">
+          <p class="card__price_text">Price:</p>
+          <p class="card__price_count">$2,199</p>
+        </div>
+        <a
+          href="#"
+          class="card__button"
+        >
+          BUY
+        </a>
+      </div>


The product card elements are repeated multiple times with identical content. Consider using a templating engine or JavaScript to dynamically generate these cards to avoid redundancy and improve maintainability.

mateacademy-ai-mentor · 2024-11-24T23:37:28Z

src/styles/index.scss

+.is-active::after {
+  content: '';
+  position: absolute;
+  bottom: -19;


The value for 'bottom' should include a unit (e.g., 'px'). Without a unit, this might not work as expected in some browsers.

mateacademy-ai-mentor · 2024-11-24T23:37:28Z

src/styles/index.scss

+.card:hover {
+  transform: scale(1.2);


The scale transformation on hover might be too large (1.2), which could affect the layout or cause overlap with other elements. Consider using a smaller scale value to ensure a smoother user experience.

mateacademy-ai-mentor

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

There are no files to review. Please make sure that you have added files to the pull request. Some files and directories may be ignored from the review or failed to load.
_{[CODE: 4]}

Additional info:
Ignored files list:

src/index.html
src/styles/index.scss

If the issue is within your control, please address it.
The next review attempt will automatically launch in 1 hour.
If you believe this is an error or need assistance, please contact the support team.

mateacademy-ai-mentor

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

There are no files to review. Please make sure that you have added files to the pull request. Some files and directories may be ignored from the review or failed to load.
_{[CODE: 4]}

Additional info:
Ignored files list:

src/index.html
src/styles/index.scss

If the issue is within your control, please address it.
The next review attempt will automatically launch in 1 hour.
If you believe this is an error or need assistance, please contact the support team.

mateacademy-ai-mentor

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

There are no files to review. Please make sure that you have added files to the pull request. Some files and directories may be ignored from the review or failed to load.
_{[CODE: 4]}

Additional info:
Ignored files list:

src/index.html
src/styles/index.scss

If the issue is within your control, please address it.
The next review attempt will automatically launch in 1 hour.
If you believe this is an error or need assistance, please contact the support team.

mateacademy-ai-mentor

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

There are no files to review. Please make sure that you have added files to the pull request. Some files and directories may be ignored from the review or failed to load.
_{[CODE: 4]}

Additional info:
Ignored files list:

src/index.html
src/styles/index.scss

If the issue is within your control, please address it.
The next review attempt will automatically launch in 1 hour.
If you believe this is an error or need assistance, please contact the support team.

TarasHoliuk

GJ 👍 Added a few comments

TarasHoliuk · 2024-12-08T09:24:07Z

src/styles/index.scss

+
+.card__code {
+  font-size: 10px;
+  font-weight: 400;


400 is a default font-weight value - no need to set it everywhere

TarasHoliuk · 2024-12-08T09:25:14Z

src/styles/index.scss

+  margin: 0;
+}
+
+.card {


move each BEM block to a separate file

Also, use SCSS nesting

TarasHoliuk · 2024-12-08T09:26:10Z

src/styles/index.scss

+  --blue-color: #00acdc;
+  --black-color: #060b35;


You've already defined such colors in SCSS variables above

TarasHoliuk · 2024-12-08T09:27:34Z

src/styles/index.scss

+.header {
+  display: flex;
+  justify-content: end;
+  gap: 243px;


What exactly can be simplified here?

remove the gap and change the justify-content to space-between

TarasHoliuk · 2024-12-08T09:27:55Z

src/styles/index.scss

+  margin: 0;
+}
+
+.nav__link-list {


Suggested change

.nav__link-list {

.nav__list {

volodymyr-soltys97

Good job!
Let's improve your code

volodymyr-soltys97 · 2024-12-11T08:32:51Z

src/index.html

+          <li class="nav__item">
+            <a
+              class="nav__link is-active"
+              href="#"
+            >
+              Apple
+            </a>
+          </li>
+          <li class="nav__link">Samsung</li>


Every word should be a link, fix it everywhere

Suggested change

<li class="nav__item">

<a

class="nav__link is-active"

href="#"

>

Apple

</a>

</li>

<li class="nav__link">Samsung</li>

<li class="nav__item">

<a

class="nav__link is-active"

href="#apple"

>

Apple

</a>

</li>

<li class="nav__item">

<a class="nav__link" href="#samsung">Samsung</a>

</li>

volodymyr-soltys97 · 2024-12-11T08:33:22Z

src/styles/index.scss

+$primary-color: #00acdc;
+$font-color: #060b35;
+$light-grey: #616070;
+$font-family: 'Roboto', sans-serif;


Move variables to the separate file

How to do it?

volodymyr-soltys97 · 2024-12-11T08:33:32Z

src/styles/index.scss

+  margin: 0;
+  min-width: 1024px;


Suggested change

margin: 0;

min-width: 1024px;

volodymyr-soltys97 · 2024-12-11T08:33:44Z

src/styles/index.scss

+  justify-content: end;
+  gap: 243px;
+  height: 60px;
+  background-color: #fff;


This color is default

Suggested change

background-color: #fff;

volodymyr-soltys97 · 2024-12-11T08:34:01Z

src/styles/index.scss

+.nav__list {
+  display: flex;
+  gap: 20px;
+  list-style-type: none;
+  margin: 0;
+  padding: 0;
+}
+
+.nav__link {
+  color: #060b35;
+  cursor: pointer;
+  text-transform: uppercase;
+  transition: color 0.3s;
+  position: relative;
+  line-height: 60px;
+  display: block;
+  height: 60px;
+}
+
+.nav {
+  display: flex;
+  align-items: center;
+}
+
+.nav__link:hover {
+  color: $primary-color;
+}
+
+.is-active {


Move header styles to the separate file

volodymyr-soltys97 · 2024-12-11T08:34:18Z

src/styles/index.scss

+.container {
+  display: grid;
+  grid-template-columns: repeat(1, 200px);
+  gap: 46px 48px;
+  justify-content: center;
+  padding: 50px 40px;
+
+  @media (min-width: 488px) {
+    grid-template-columns: repeat(2, 200px);
+  }
+
+  @media (min-width: 768px) {
+    grid-template-columns: repeat(3, 200px);
+  }
+
+  @media (min-width: 1024px) {
+    grid-template-columns: repeat(4, 200px);
+  }
+}


Move container styles to the separate file

volodymyr-soltys97 · 2024-12-11T08:34:54Z

src/styles/index.scss

+  border-radius: 5px;
+  width: 200px;
+  margin: 0;
+  padding: 32px 16px 16px;
+  font-family: $font-family;
+  border: 1px solid #f3f3f3;
+  transition:
+    transform 0.3s ease,
+    color 0.3s ease;
+}
+
+.card__image {
+  width: 160px;
+  height: 134px;
+  margin: 0 auto 40px;
+  display: block;
+}
+
+.card__title {
+  font-size: 12px;
+  font-weight: 500;
+  line-height: 18px;
+  color: $font-color;
+  margin-bottom: 4px;
+  transition: color 0.3s ease;
+}


Move these styles to the separate card file styles styles, and use scss nesting everywhere

volodymyr-soltys97 · 2024-12-11T08:35:13Z

src/styles/index.scss

+.stars {
+  display: flex;
+  gap: 4px;
+  padding-left: 1px;
+  align-items: center;
+}
+
+.stars__star {
+  background-image: url(../images/star.svg);
+  width: 16px;
+  height: 16px;
+  background-repeat: no-repeat;
+  background-position: center;
+}
+


Move stars styles to the separate file

volodymyr-soltys97 · 2024-12-11T08:35:45Z

src/index.html

+          <div class="stars">
+            <div class="stars__star-active"></div>
+            <div class="stars__star-active"></div>
+            <div class="stars__star-active"></div>
+            <div class="stars__star-active"></div>
+            <div class="stars__star"></div>
+          </div>


Fix it everywhere

Suggested change

<div class="stars">

<div class="stars__star-active"></div>

<div class="stars__star-active"></div>

<div class="stars__star-active"></div>

<div class="stars__star-active"></div>

<div class="stars__star"></div>

</div>

<div class="stars stars--4">

<div class="stars__star"></div>

<div class="stars__star"></div>

<div class="stars__star"></div>

<div class="stars__star"></div>

<div class="stars__star"></div>

</div>

etojeDenys · 2024-12-15T09:27:29Z

src/styles/index.scss

+.header {
+  display: flex;
+  justify-content: end;
+  gap: 243px;


remove the gap and change the justify-content to space-between

etojeDenys · 2024-12-15T09:27:44Z

src/index.html

+          <div class="stars">
+            <div class="stars__star-active"></div>
+            <div class="stars__star-active"></div>
+            <div class="stars__star-active"></div>
+            <div class="stars__star-active"></div>
+            <div class="stars__star"></div>
+          </div>


add solution

4aee23d

mateacademy-ai-mentor suggested changes Nov 24, 2024

View reviewed changes

Knysh19 added 6 commits December 4, 2024 20:04

add solution

cd2e610

add solution

4df5ee7

add solution

aa749cc

add solution

09bf5e8

add solution

e528285

add solution

5fee25a

Knysh19 requested a review from mateacademy-ai-mentor December 7, 2024 22:03

mateacademy-ai-mentor reviewed Dec 7, 2024

View reviewed changes

mateacademy-ai-mentor reviewed Dec 8, 2024

View reviewed changes

TarasHoliuk suggested changes Dec 8, 2024

View reviewed changes

add solution

fac84f8

Knysh19 requested a review from TarasHoliuk December 10, 2024 16:25

volodymyr-soltys97 suggested changes Dec 11, 2024

View reviewed changes

add solution

efe60f6

Knysh19 requested a review from volodymyr-soltys97 December 14, 2024 16:48

etojeDenys suggested changes Dec 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add solution #4869

add solution #4869

Knysh19 commented Nov 24, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Nov 24, 2024

mateacademy-ai-mentor Nov 24, 2024

mateacademy-ai-mentor Nov 24, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor left a comment

TarasHoliuk left a comment

TarasHoliuk Dec 8, 2024

TarasHoliuk Dec 8, 2024

TarasHoliuk Dec 8, 2024

TarasHoliuk Dec 8, 2024

Knysh19 Dec 10, 2024

etojeDenys Dec 15, 2024

TarasHoliuk Dec 8, 2024

volodymyr-soltys97 left a comment

volodymyr-soltys97 Dec 11, 2024

volodymyr-soltys97 Dec 11, 2024

Knysh19 Dec 11, 2024

volodymyr-soltys97 Dec 11, 2024

volodymyr-soltys97 Dec 11, 2024

volodymyr-soltys97 Dec 11, 2024

volodymyr-soltys97 Dec 11, 2024

volodymyr-soltys97 Dec 11, 2024

volodymyr-soltys97 Dec 11, 2024

volodymyr-soltys97 Dec 11, 2024

etojeDenys Dec 15, 2024

etojeDenys Dec 15, 2024

etojeDenys Dec 15, 2024

add solution #4869

Are you sure you want to change the base?

add solution #4869

Conversation

Knysh19 commented Nov 24, 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

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

TarasHoliuk 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

volodymyr-soltys97 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment