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

add task solution catalog #4998

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

add task solution catalog #4998

wants to merge 2 commits into from

Conversation

nataliaso23
Copy link

mateacademy-ai-mentor
mateacademy-ai-mentor suggested changes Jan 11, 2025
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.

Hello! 😊 It looks like you've made great progress, but there are a few critical issues that need to be addressed before we can approve the pull request:

  1. SCSS Variables: Ensure that all necessary SCSS variables, such as $color-blue-accent, $color-white, $text-secondary-color, $text-main-color, and $color-gray, are defined. Missing definitions can lead to compilation errors, which are critical to the functionality of your stylesheets.

  2. BEM and HTML Structure: Make sure that the HTML structure consistently follows BEM naming conventions. This is important for maintainability and readability.

  3. Data Attributes: Correct the data-qa attribute in the HTML file to match the task requirements. It should be data-qa="card-hover" for the link Buy inside the first card.

  4. Maintainability: The repeated content in the product cards should be addressed. Consider using a templating approach or JavaScript to dynamically generate these cards, which will improve maintainability and reduce redundancy.

Once these issues are resolved, your code will be in great shape! Keep up the good work, and feel free to reach out if you have any questions or need further assistance. 🚀

✨ 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 Outdated
Comment on lines 148 to 384
class="card"
data-qa="card"
>
<img
class="card__img"
src="./images/imac.jpeg"
alt=" APPLE A1419 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__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__cost">
<p class="card__price">Price:</p>
<p class="card__total">$2,199</p>
</div>
<button
data-qa="hover"
class="card__button"
>
Buy
</button>
</div>
<div
class="card"
data-qa="card"
>
<img
class="card__img"
src="./images/imac.jpeg"
alt=" APPLE A1419 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__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__cost">
<p class="card__price">Price:</p>
<p class="card__total">$2,199</p>
</div>
<button
data-qa="hover"
class="card__button"
>
Buy
</button>
</div>
<div
class="card"
data-qa="card"
>
<img
class="card__img"
src="./images/imac.jpeg"
alt=" APPLE A1419 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__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__cost">
<p class="card__price">Price:</p>
<p class="card__total">$2,199</p>
</div>
<button
data-qa="hover"
class="card__button"
>
Buy
</button>
Copy link

Choose a reason for hiding this comment

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

The product card content is repeated multiple times with the same details. Consider using a templating approach or JavaScript to dynamically generate these cards to improve maintainability and reduce redundancy.

src/styles/blocks/card.scss
}

.card {
border: 1px solid $color-gray;
Copy link

Choose a reason for hiding this comment

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

Ensure that the SCSS variable $color-gray is defined in your stylesheets. If it's not defined, it will cause a compilation error.

src/styles/blocks/card.scss

&__title {
width: 100%;
color: $text-main-color;
Copy link

Choose a reason for hiding this comment

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

Ensure that the SCSS variable $text-main-color is defined. If it's not defined, it will cause a compilation error.

src/styles/blocks/card.scss
}
}
&__code {
color: $text-secondary-color;
Copy link

Choose a reason for hiding this comment

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

Ensure that the SCSS variable $text-secondary-color is defined. If it's not defined, it will cause a compilation error.

src/styles/blocks/card.scss
Comment on lines +103 to +104
color: $color-white;
background-color: $color-blue-accent;
Copy link

Choose a reason for hiding this comment

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

Ensure that the SCSS variables $color-white and $color-blue-accent are defined. If they are not defined, it will cause a compilation error.

src/styles/blocks/header.scss
}

.nav__link:hover {
color: $color-blue-accent;
Copy link

Choose a reason for hiding this comment

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

Ensure that the SCSS variable $color-blue-accent is defined. If it's not defined, it will cause a compilation error. Consider using the CSS variable var(--blue-color) for consistency if this variable is intended to be the same.

mateacademy-ai-mentor
mateacademy-ai-mentor approved these changes Jan 11, 2025
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.

Great job on your submission! 🎉 While there are some areas for improvement, your effort is commendable. Here are a few key points to focus on:

  1. SCSS Variables: Ensure that all SCSS variables, such as $color-gray, $text-main-color, $text-secondary-color, $color-white, and $color-blue-accent, are defined to avoid compilation errors.

  2. BEM Naming Convention: Your HTML structure generally follows BEM conventions, which is excellent for maintainability. Ensure all elements and modifiers are consistently named according to BEM principles.

  3. Data Attributes: The data-qa attribute for the 'Buy' button inside the first card should be data-qa="card-hover". Make sure this is consistent across all cards.

  4. Repeated Content: Consider using a templating approach or JavaScript to dynamically generate product cards to reduce redundancy and improve maintainability.

Your decision to move forward with the project is to encourage learning and improvement. Please review the comments and address these issues in future iterations to enhance your code quality. Keep up the good work and continue to learn from these experiences! 🚀

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

src/index.html
</div>

<button
data-qa="hover"
Copy link

Choose a reason for hiding this comment

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

The data-qa attribute here should be data-qa="card-hover" to match the task requirements for the 'Buy' button inside the first card.

src/index.html
</div>

<button
data-qa="hover"
Copy link

Choose a reason for hiding this comment

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

The data-qa attribute here should be data-qa="card-hover" to match the task requirements for the 'Buy' button inside the first card.

src/index.html
</div>

<button
data-qa="hover"
Copy link

Choose a reason for hiding this comment

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

The data-qa attribute here should be data-qa="card-hover" to match the task requirements for the 'Buy' button inside the first card.

src/index.html
</div>

<button
data-qa="hover"
Copy link

Choose a reason for hiding this comment

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

The data-qa attribute here should be data-qa="card-hover" to match the task requirements for the 'Buy' button inside the first card.

src/index.html
</div>

<button
data-qa="hover"
Copy link

Choose a reason for hiding this comment

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

The data-qa attribute here should be data-qa="card-hover" to match the task requirements for the 'Buy' button inside the first card.

src/styles/blocks/card.scss
}

&__reviews {
color: $text-main-color;
Copy link

Choose a reason for hiding this comment

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

Ensure that the SCSS variable $text-main-color is defined. Missing variable definitions can lead to compilation errors.

src/styles/blocks/card.scss
margin-bottom: 16px;
}
&__price {
color: $text-secondary-color;
Copy link

Choose a reason for hiding this comment

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

Ensure that the SCSS variable $text-secondary-color is defined. Missing variable definitions can lead to compilation errors.

src/styles/blocks/card.scss
border: 1px solid #00acdc;

&:hover {
color: $color-white;
Copy link

Choose a reason for hiding this comment

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

Ensure that the SCSS variable $color-white is defined. Missing variable definitions can lead to compilation errors.

src/styles/blocks/card.scss

&:hover {
color: $color-white;
background-color: $color-blue-accent;
Copy link

Choose a reason for hiding this comment

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

Ensure that the SCSS variable $color-blue-accent is defined. Missing variable definitions can lead to compilation errors.

src/styles/blocks/header.scss
}

.nav__link:hover {
color: $color-blue-accent;
Copy link

Choose a reason for hiding this comment

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

Ensure that the SCSS variable $color-blue-accent is defined. Missing variable definitions can lead to compilation errors.

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
@nataliaso23 @mateacademy-ai-mentor