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.

Sign up for GitHub

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

Jump to bottom

created catalog #4830

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

created catalog #4830

wants to merge 2 commits into from

Conversation

vikapazyuk
Copy link

mateacademy-ai-mentor
mateacademy-ai-mentor suggested changes Nov 7, 2024
View reviewed changes
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.

Thank you for your hard work! 😊 There are a few critical issues that need to be addressed before we can approve your PR. The repeated content within the <div class="card"> elements indicates a lack of dynamic content generation, which is crucial for maintainability and scalability. Additionally, there are minor issues with the SCSS, such as a typo in the transition property and unnecessary repetition. Please address these issues to meet the core requirements of the task. Keep up the great work, and I look forward to your improvements! 🚀

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

src/index.html
Comment on lines +149 to +427
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
</div>
<p class="card__reviews">Reviews: 5</p>
</div>

<div class="card__price">
<p class="card__name-price">Price:</p>
<p class="card__num-price">$2,199</p>
</div>

<button
class="card__button"
data-qa="card-hover"
type="button"
>
Buy
</button>
</div>

<div
class="card"
data-qa="card"
>
<img
class="card__img"
src="./images/imac.jpeg"
alt="imac image"
/>

<p class="card__title">
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</p>
<p class="card__product-code">Product code: 195434</p>

<div class="card__stars">
<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>
<p class="card__reviews">Reviews: 5</p>
</div>

<div class="card__price">
<p class="card__name-price">Price:</p>
<p class="card__num-price">$2,199</p>
</div>

<button
class="card__button"
data-qa="card-hover"
type="button"
>
Buy
</button>
</div>

<div
class="card"
data-qa="card"
>
<img
class="card__img"
src="./images/imac.jpeg"
alt="imac image"
/>

<p class="card__title">
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</p>
<p class="card__product-code">Product code: 195434</p>

<div class="card__stars">
<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>
<p class="card__reviews">Reviews: 5</p>
</div>

<div class="card__price">
<p class="card__name-price">Price:</p>
<p class="card__num-price">$2,199</p>
</div>

<button
class="card__button"
data-qa="card-hover"
type="button"
>
Buy
</button>
</div>
Copy link

Choose a reason for hiding this comment

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

The content within each <div class="card"> is repeated multiple times with the same data. This redundancy suggests that the content is not being dynamically generated. Consider using a templating engine or JavaScript to dynamically populate these cards with unique data to improve maintainability and scalability.

src/styles/card.scss Outdated
Comment on lines 14 to 15
transform: scale(1.2);
transition: 0.3s;
Copy link

Choose a reason for hiding this comment

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

The transition property is repeated in the &:hover selector. You can remove the transition: 0.3s; line as the transition: all 0.3s; is already defined in the .card class.

src/styles/card.scss Outdated
Comment on lines 32 to 34
&:hover .cart__title {
color: #34568b;
transition: 0, 3s;
Copy link

Choose a reason for hiding this comment

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

There is a typo in the transition property. It should be transition: 0.3s; instead of transition: 0, 3s;. This will ensure the hover effect on the &__title works smoothly.

mateacademy-ai-mentor
mateacademy-ai-mentor approved these changes Nov 7, 2024
View reviewed changes
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.

Review Summary

Great job on implementing the HTML and SCSS files! 🎉 Your code meets most of the task requirements and constraints, and you've done well with the following:

  • HTML Structure: The use of semantic elements like <header>, <nav>, and <main> is correct, ensuring a well-structured document.
  • SCSS Organization: Your SCSS files are modular and well-organized, making use of flexbox and grid for layout, which aligns with modern web design practices.
  • Responsive Design: The use of CSS Grid and media queries for responsive design is commendable.
  • Interactivity: Smooth hover effects and clear styling rules enhance the user experience.

However, there are a few areas to improve:

  1. BEM and SCSS Consistency: Ensure that all components strictly follow BEM conventions and that SCSS is used consistently across all styles.
  2. Content Repetition: Consider using unique content for each product card to avoid repetition unless it's intentional for testing purposes.

Your implementation is mostly correct, and the decision to approve is made to help you move forward with your learning. Please review the comments to improve your code. Addressing these issues will enhance your skills and help avoid similar problems in the future. Keep up the great work and continue learning from your experiences! 🚀

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

src/index.html
@@ -21,7 +21,411 @@
/>
</head>

<body>
<h1>Catalog</h1>
<body class="body">
Copy link

Choose a reason for hiding this comment

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

The <body> tag is correctly opened with a class attribute.

src/index.html
<body>
<h1>Catalog</h1>
<body class="body">
<header class="header">
Copy link

Choose a reason for hiding this comment

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

The <header> tag is correctly used to contain the logo and navigation.

src/index.html
</nav>
</header>

<main class="main">
Copy link

Choose a reason for hiding this comment

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

The <main> tag is used appropriately to contain the main content of the page.

src/index.html
</header>

<main class="main">
<div class="main__container">
Copy link

Choose a reason for hiding this comment

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

Consider using unique content for each product card to avoid repetition unless this is intentional for layout testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mateacademy-ai-mentor mateacademy-ai-mentor mateacademy-ai-mentor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vikapazyuk @mateacademy-ai-mentor