add task solution #5023

fwrwxx · 2024-11-28T22:54:02Z

mateacademy-ai-mentor

Thank you for your hard work on this project! There are a few critical issues that need to be addressed before we can approve the pull request. The use of an anchor tag inside a button element is not semantically correct and could lead to accessibility issues. Additionally, the SCSS file linking and BEM block requirements are not fully met, which are core requirements for the task. Please make these changes, and feel free to reach out if you need any help! 😊 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-28T22:54:53Z

src/index.html

+      <button
+        class="buy-button"
+        data-qa="hover"
+      >
+        <a
+          href="#"
+          data-qa="hover"
+        >
+          Buy
+        </a>
+      </button>


Using an anchor tag inside a button element is not semantically correct and can lead to accessibility issues. Consider using either a button element with a click event or an anchor tag styled as a button.

mateacademy-ai-mentor · 2024-11-28T22:54:53Z

src/index.html

+    >
+      <img
+        class="img"
+        src="./images/imac.jpeg"


Ensure that the image path './images/imac.jpeg' is correct and that the image file exists at the specified location.

mateacademy-ai-mentor · 2024-11-28T22:54:53Z

src/styles/index.scss

+  & .price {
+    color: $secondary-color;
+    font-size: 12px;
+    margin: 0;
+  }


Using the same class name 'price' for both the div and the paragraph can cause confusion and styling conflicts. Consider renaming one of these classes to avoid potential issues.

mateacademy-ai-mentor · 2024-11-28T22:54:53Z

src/styles/index.scss

@@ -1,3 +1,127 @@
+@import './variables';


Ensure that the './variables' file exists and is correctly referenced. This is important for the SCSS to compile successfully.

mateacademy-ai-mentor

Thank you for your submission! 🎉 While your code is generally well-structured and demonstrates a good understanding of SCSS and HTML, there are a few areas that need attention:

Class Name Confusion: The use of the class name price for both a div and a paragraph could lead to confusion and styling issues. Consider using more specific class names to differentiate between the two elements.
Typo in Class Name: The class name discribe appears to be a typo. Please correct it to describe to maintain consistency and avoid confusion.
SCSS File Linking: Although the SCSS file is correctly linked due to the use of the Parcel library, remember that browsers do not understand SCSS directly. Ensure that the SCSS is compiled to a CSS file for broader compatibility.
BEM Block Requirements: The task requires separating BEM blocks card and stars into individual files. This separation is crucial for maintaining a clean and organized codebase.
Redundant Files: The presence of both index.css and index.scss files is unnecessary. Since SCSS is being used, consider removing the CSS file to avoid redundancy.

These points are crucial for the functionality and maintainability of your code. Please address them in future submissions to enhance your coding skills. Keep up the good work, and continue learning and improving! 🚀

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

mateacademy-ai-mentor · 2024-11-29T12:50:40Z

src/index.html

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


The stylesheet is linked as an SCSS file. Browsers do not understand SCSS directly; it needs to be compiled to CSS first. Ensure that the SCSS is compiled to a CSS file and link that CSS file here.

mateacademy-ai-mentor · 2024-11-29T12:50:40Z

src/index.html

+      <div class="price">
+        <p class="price">Price:</p>


The class name 'price' is used for both the div and the paragraph. This can lead to confusion and potential styling issues. Consider using a more specific class name for the paragraph to differentiate it from the div.

mateacademy-ai-mentor · 2024-11-29T12:50:40Z

src/styles/index.css

+  height: 134px;
+}
+
+.discribe {


The class name 'discribe' appears to be a typo. Consider changing it to 'describe' to avoid confusion and maintain consistency.

mateacademy-ai-mentor · 2024-11-29T12:50:40Z

src/styles/index.css

+.price {
+  display: flex;
+  align-items: center;
+  justify-content: space-between;
+  line-height: 18px;
+  height: 18px;
+  width: 100%;
+  margin-bottom: 16px;
+  margin-top: 24px;
+}
+
+.price .price {
+  color: #616070;
+  font-size: 12px;
+  margin: 0;
+}


The class name 'price' is used for both a div and a paragraph, which can lead to confusion and potential styling issues. Consider using a more specific class name for the paragraph to differentiate it from the div.

mateacademy-ai-mentor · 2024-11-29T12:50:41Z

src/styles/index.scss

+  height: 134px;
+}
+
+.discribe {


The class name 'discribe' appears to be a typo. Consider changing it to 'describe' to avoid confusion and maintain consistency.

mateacademy-ai-mentor · 2024-11-29T12:50:41Z

src/styles/index.scss

+.price {
+  display: flex;
+  align-items: center;
+  justify-content: space-between;
+  line-height: 18px;
+  height: 18px;
+  width: 100%;
+  margin-bottom: 16px;
+  margin-top: 24px;
+
+  .price {
+    color: $secondary-color;
+    font-size: 12px;
+    margin: 0;
+  }


The class name 'price' is used for both a div and a paragraph, which can lead to confusion and potential styling issues. Consider using a more specific class name for the paragraph to differentiate it from the div.

VitaliyBondarenko1982

Good job.
Please check comments.

VitaliyBondarenko1982 · 2024-11-29T13:40:29Z

src/styles/index.scss

+  align-items: center;
+  border: 1px solid $button-color;
+  border-radius: 5px;
+  width: 164px;


do not hardcode width in this case

Suggested change

width: 164px;

width: 100%;

VitaliyBondarenko1982 · 2024-11-29T13:41:51Z

src/styles/index.scss

+  border: 1px solid $button-color;
+  border-radius: 5px;
+  width: 164px;
+  height: 38px;


use paddings instead hardcoded height. vertical paddings + line-height + borders should make height 40px

VitaliyBondarenko1982 · 2024-11-29T13:43:08Z

src/index.html

+      <h3 class="describe">
+        APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
+      </h3>
+      <p class="code">Product code: 195434</p>


follow bem convention in class naming.
card__describe, card__code, card__img...

VitaliyBondarenko1982 · 2024-11-29T13:44:35Z

src/styles/index.scss

+
+.card {
+  width: 200px;
+  height: 406px;


do not hardcode height. content card should make height for card.
check design. card should have border. total card width in design 200px, total height - 408px

Suggested change

height: 406px;

add task solution

a17985e

mateacademy-ai-mentor suggested changes Nov 28, 2024

View reviewed changes

add task solution

694d93b

fwrwxx requested a review from mateacademy-ai-mentor November 29, 2024 12:49

mateacademy-ai-mentor approved these changes Nov 29, 2024

View reviewed changes

add task solution

d24c8c3

fwrwxx requested a review from mateacademy-ai-mentor November 29, 2024 13:06

VitaliyBondarenko1982 suggested changes Nov 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add task solution #5023

add task solution #5023

fwrwxx commented Nov 28, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Nov 28, 2024

mateacademy-ai-mentor Nov 28, 2024

mateacademy-ai-mentor Nov 28, 2024

mateacademy-ai-mentor Nov 28, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Nov 29, 2024

mateacademy-ai-mentor Nov 29, 2024

mateacademy-ai-mentor Nov 29, 2024

mateacademy-ai-mentor Nov 29, 2024

mateacademy-ai-mentor Nov 29, 2024

mateacademy-ai-mentor Nov 29, 2024

VitaliyBondarenko1982 left a comment

VitaliyBondarenko1982 Nov 29, 2024

VitaliyBondarenko1982 Nov 29, 2024

VitaliyBondarenko1982 Nov 29, 2024

VitaliyBondarenko1982 Nov 29, 2024

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

add task solution #5023

Are you sure you want to change the base?

add task solution #5023

Conversation

fwrwxx commented Nov 28, 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

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

VitaliyBondarenko1982 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