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 #4829

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

add task solution #4829

wants to merge 2 commits into from

Conversation

GuilhermeSant
Copy link

  • [x ] All components follow BEM and use SCSS
  • [ x] Repeated sizes and special colors are put to variables
  • [ x] Grid is used for the columns
  • [ x] Cards are shown in 1, 2, 3, or 4 columns based on screen resolution
  • [ x] All changes on :hover are smooth
  • [ x] Code follows all the Code Style Rules ❗️

mateacademy-ai-mentor
mateacademy-ai-mentor suggested changes Nov 6, 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 on this project! There are a few critical issues that need to be addressed before we can approve the pull request:

  1. SCSS Compilation: The stylesheet link in your HTML is pointing to a SCSS file. Browsers cannot interpret SCSS directly, so you'll need to compile it into a CSS file and update the link accordingly.

  2. Grid Column Variable Scope: The $column-count variable is defined inside a media query, making it local to that scope. This can lead to unexpected behavior. Consider defining it outside the media queries for global accessibility.

  3. Undefined Variables and Mixins: Ensure that all SCSS variables and mixins, such as $button-color, $prod-img-source, and smooth, are defined to avoid compilation errors.

  4. Repeated Product Details: The product details are repeated multiple times. If this is for testing, it's fine, but ensure unique data for each product card in a production environment.

Please address these issues to ensure the functionality and maintainability of your code. Keep up the great work, and feel free to reach out if you have any questions! 😊

✨ 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
@@ -17,11 +17,256 @@
/>
<link
rel="stylesheet"
href="styles/index.scss"
href="styles/main.scss"
Copy link

Choose a reason for hiding this comment

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

The stylesheet link is pointing to a SCSS file (styles/main.scss). Browsers cannot directly interpret SCSS files. You need to compile the SCSS into CSS and link to the resulting CSS file instead.

src/index.html
Comment on lines +88 to +268
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</div>
<div class="card__prod-code">Product code: 195434</div>

<!--product reviews and stars avaliation-->
<section class="card__reviews">
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-text">Reviews: 5</div>
</section>

<!--product price-->
<section class="card__price">
<div>Price:</div>
<div class="card__price-value">$2,199</div>
</section>

<!--button buy-->
<button class="card__button-buy">BUY</button>
</div>
<div class="card">
<!--Product name and code-->
<div class="card__text">
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</div>
<div class="card__prod-code">Product code: 195434</div>

<!--product reviews and stars avaliation-->
<section class="card__reviews">
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-text">Reviews: 5</div>
</section>

<!--product price-->
<section class="card__price">
<div>Price:</div>
<div class="card__price-value">$2,199</div>
</section>

<!--button buy-->
<button class="card__button-buy">BUY</button>
</div>
<div class="card">
<!--Product name and code-->
<div class="card__text">
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</div>
<div class="card__prod-code">Product code: 195434</div>

<!--product reviews and stars avaliation-->
<section class="card__reviews">
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-text">Reviews: 5</div>
</section>

<!--product price-->
<section class="card__price">
<div>Price:</div>
<div class="card__price-value">$2,199</div>
</section>

<!--button buy-->
<button class="card__button-buy">BUY</button>
</div>
<div class="card">
<!--Product name and code-->
<div class="card__text">
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</div>
<div class="card__prod-code">Product code: 195434</div>

<!--product reviews and stars avaliation-->
<section class="card__reviews">
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-text">Reviews: 5</div>
</section>

<!--product price-->
<section class="card__price">
<div>Price:</div>
<div class="card__price-value">$2,199</div>
</section>

<!--button buy-->
<button class="card__button-buy">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 details, including name, code, reviews, and price, are repeated multiple times with the same content. If this is intentional for layout testing, it is fine. However, for a real application, ensure that each product card displays unique data.

src/styles/blocks/card.scss
@@ -0,0 +1,98 @@
.card {
@include smooth;
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 smooth mixin is defined in your SCSS files. If it's not, you need to define it to avoid compilation errors.

src/styles/blocks/card.scss
@include smooth;

border: solid 1px #f3f3f3;
width: $card-width;
Copy link

Choose a reason for hiding this comment

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

Make sure that the $card-width variable is defined in your SCSS files. If it's not, you need to define it to avoid compilation errors.

src/styles/blocks/card.scss

border: solid 1px #f3f3f3;
width: $card-width;
height: $card-height;
Copy link

Choose a reason for hiding this comment

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

Make sure that the $card-height variable is defined in your SCSS files. If it's not, you need to define it to avoid compilation errors.

src/styles/blocks/catalog.scss

@media (min-width: 1024px) {
& {
$column-count: 4;
Copy link

Choose a reason for hiding this comment

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

Defining the $column-count variable inside a media query makes it local to that scope. Consider defining it outside the media queries to ensure it's accessible globally and can be reused.

src/styles/blocks/catalog.scss
padding: 50px 40px;

display: grid;
grid-template-columns: repeat($column-count, $column-width);
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 $column-width variable is defined in your SCSS files. If it's not, you need to define it to avoid compilation errors.

src/styles/blocks/header.scss
}

&-link {
@include smooth;
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 smooth mixin is defined in your SCSS files. If it's not, you need to define it to avoid compilation errors.

src/styles/blocks/header.scss
display: flex;
flex-direction: row;
padding-right: 50px;
margin-left: 243px;
Copy link

Choose a reason for hiding this comment

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

Using a fixed margin-left value (243px) might affect the responsiveness of the layout. Consider using relative units or media queries to adjust this value for different screen sizes.

src/styles/blocks/header.scss

@media (min-width: 1200px) {
& {
margin-left: 419px;
Copy link

Choose a reason for hiding this comment

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

Using a fixed margin-left value (419px) might affect the responsiveness of the layout. Consider using relative units or media queries to adjust this value for different screen sizes.

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

Great job on your submission! 🎉 While the code meets most of the task requirements, there are a few areas for improvement. Ensure that all SCSS variables and mixins are defined to avoid compilation errors. Consider using CSS custom properties for dynamic values in media queries. Address the use of :nth-child selectors for more robust styling, and ensure image paths are correct to prevent broken links. The decision to approve is to help you move forward with your learning. Please review the comments to enhance your code quality and maintainability. Keep up the good work and continue improving! 💪

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

src/index.html
Comment on lines +25 to +26
<a
href="#"
Copy link

Choose a reason for hiding this comment

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

The 'href' attribute is set to '#', which is a placeholder. Consider replacing it with a valid URL or removing it if the link is not needed.

src/index.html
Comment on lines +53 to +269
</div>
<div class="card__prod-code">Product code: 195434</div>

<!--product reviews and stars avaliation-->
<section class="card__reviews">
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-text">Reviews: 5</div>
</section>

<!--product price-->
<section class="card__price">
<div>Price:</div>
<div class="card__price-value">$2,199</div>
</section>

<!--button buy-->
<button class="card__button-buy">BUY</button>
</div>
<div class="card">
<!--Product name and code-->
<div class="card__text">
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</div>
<div class="card__prod-code">Product code: 195434</div>

<!--product reviews and stars avaliation-->
<section class="card__reviews">
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-text">Reviews: 5</div>
</section>

<!--product price-->
<section class="card__price">
<div>Price:</div>
<div class="card__price-value">$2,199</div>
</section>

<!--button buy-->
<button class="card__button-buy">BUY</button>
</div>
<div class="card">
<!--Product name and code-->
<div class="card__text">
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</div>
<div class="card__prod-code">Product code: 195434</div>

<!--product reviews and stars avaliation-->
<section class="card__reviews">
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-text">Reviews: 5</div>
</section>

<!--product price-->
<section class="card__price">
<div>Price:</div>
<div class="card__price-value">$2,199</div>
</section>

<!--button buy-->
<button class="card__button-buy">BUY</button>
</div>
<div class="card">
<!--Product name and code-->
<div class="card__text">
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</div>
<div class="card__prod-code">Product code: 195434</div>

<!--product reviews and stars avaliation-->
<section class="card__reviews">
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-star"></div>
<div class="card__reviews-text">Reviews: 5</div>
</section>

<!--product price-->
<section class="card__price">
<div>Price:</div>
<div class="card__price-value">$2,199</div>
</section>

<!--button buy-->
<button class="card__button-buy">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 product details are repeated multiple times with the same content. Ensure this repetition is intentional for testing purposes. In a real-world scenario, each product card should have unique content.

src/styles/blocks/card.scss
Comment on lines +1 to +98
.card {
@include smooth;

border: solid 1px #f3f3f3;
width: $card-width;
height: $card-height;

background-image: url($prod-img-source);
background-position-y: 32px;
background-position-x: 19px;
background-size: $prod-img-size;
background-repeat: no-repeat;

&__text {
@include smooth;

line-height: 18px;
font-size: $font-size-default;
font-weight: 500;
color: #060b35;

margin-top: 206px;
padding: 0 16px;
}

&__prod-code {
color: #616070;
font-size: 10px;
padding: 4px 16px;
}

&__reviews {
display: flex;
flex-direction: row;
justify-content: space-between;

margin-top: 16px;
padding: 0 16px;

&-text {
font-size: 10px;
font-weight: 400;
line-height: 14px;

margin-left: 17px;
}
}

&__price {
font-size: $font-size-default;
font-weight: 400;
line-height: 18px;
color: #616070;

margin-top: 24px;
padding: 0 16px;
display: flex;
flex-direction: row;

&-value {
margin-left: 85px;
font-weight: 700;
font-size: 16px;
color: #000;
}
}

&__button-buy {
@include smooth;

width: $button-width;
height: $button-height;

border-width: 0;
border-radius: 5px;
background-color: $button-color;
margin: 16px;

color: $button-font-color;
font-size: 14px;
font-weight: 700;
text-align: center;

&:hover {
background-color: $button-color--hover;
color: $button-font-color--hover;
border: solid 1px #00acdc;
}
}

&:hover {
transform: scale(1.2);
}

&:hover &__text {
color: #34568b;
}
}
Copy link

Choose a reason for hiding this comment

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

Ensure that all variables (e.g., $card-width, $prod-img-source) and mixins (e.g., smooth) used in this file are defined in your SCSS setup. Undefined variables or mixins will cause compilation errors.

src/styles/blocks/catalog.scss

@media (max-width: 487px) {
& {
$column-count: 1;
Copy link

Choose a reason for hiding this comment

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

SCSS variables like $column-count are not dynamic and cannot be changed within media queries. Consider using CSS custom properties or JavaScript for dynamic values.

src/styles/blocks/catalog.scss

@media (min-width: 488px) {
& {
$column-count: 2;
Copy link

Choose a reason for hiding this comment

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

SCSS variables like $column-count are not dynamic and cannot be changed within media queries. Consider using CSS custom properties or JavaScript for dynamic values.

src/styles/main.css
Comment on lines +116 to +118
.card__reviews-star:nth-child(n+5) {
background-image: url(../images/star.svg);
}
Copy link

Choose a reason for hiding this comment

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

The use of :nth-child(n+5) assumes a fixed number of stars. Consider a more flexible approach if the number of stars can vary.

src/styles/main.css
.catalog {
padding: 50px 40px;
display: grid;
grid-template-columns: repeat(1, 200px);
Copy link

Choose a reason for hiding this comment

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

The hardcoded width of 200px in grid-template-columns could be replaced with a variable for better maintainability and flexibility.

src/styles/main.css
border: solid 1px #f3f3f3;
width: 200px;
height: 408px;
background-image: url("../images/imac.jpeg");
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 image '../images/imac.jpeg' is correctly placed in the specified path to avoid broken image links.

src/styles/main.css
}

.card__reviews-star {
background-image: url(../images/star-active.svg);
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 image '../images/star-active.svg' is correctly placed in the specified path to avoid broken image links.

src/styles/main.css
}

.card__reviews-star:nth-child(n+5) {
background-image: url(../images/star.svg);
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 image '../images/star.svg' is correctly placed in the specified path to avoid broken image links.

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