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

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

add task solution #4835

wants to merge 2 commits into from

Conversation

Linkoln223
Copy link

mateacademy-ai-mentor
mateacademy-ai-mentor suggested changes Nov 9, 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 this pull request. Specifically, the SCSS variables need to be defined to avoid compilation errors, and the grid usage for columns needs to be adjusted to meet the task requirements. Additionally, ensure that all required data-qa attributes are implemented for testing purposes. Once these issues are resolved, your code 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.

src/index.html
Comment on lines +151 to +394
alt="imac"
/>
<h2 class="card__title">
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</h2>
<span class="card__code">Product code: 195434</span>

<div class="card__rating">
<div class="stars stars--active">
<div class="stars__item"></div>
<div class="stars__item"></div>
<div class="stars__item"></div>
<div class="stars__item"></div>
<div class="stars__item"></div>
</div>
<div class="card__review">Reviews: 5</div>
</div>

<div class="card__price">
<div class="card__price-text">Price:</div>
<div class="card__price-number">$2,199</div>
</div>

<a
href="#"
class="buy"
>
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>
<span class="card__code">Product code: 195434</span>

<div class="card__rating">
<div class="stars stars--active">
<div class="stars__item"></div>
<div class="stars__item"></div>
<div class="stars__item"></div>
<div class="stars__item"></div>
<div class="stars__item"></div>
</div>
<div class="card__review">Reviews: 5</div>
</div>

<div class="card__price">
<div class="card__price-text">Price:</div>
<div class="card__price-number">$2,199</div>
</div>

<a
href="#"
class="buy"
>
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>
<span class="card__code">Product code: 195434</span>

<div class="card__rating">
<div class="stars stars--active">
<div class="stars__item"></div>
<div class="stars__item"></div>
<div class="stars__item"></div>
<div class="stars__item"></div>
<div class="stars__item"></div>
</div>
<div class="card__review">Reviews: 5</div>
</div>

<div class="card__price">
<div class="card__price-text">Price:</div>
<div class="card__price-number">$2,199</div>
</div>

<a
href="#"
class="buy"
>
Buy
</a>
</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 cards are repeated with identical content. Consider using a templating engine or JavaScript to dynamically generate these cards if the content is meant to be dynamic. This will make your code more maintainable and efficient.

src/styles/buy.scss
text-transform: uppercase;
padding: 12px 70px;
color: rgba(255, 255, 255, 1);
background-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 $blue-accent is defined in your stylesheets. If it's not defined, it will cause an error during the SCSS compilation.

src/styles/buy.scss
font-weight: 700;
font-size: 14px;
line-height: 16px;
border-radius: $rounding;
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 $rounding is defined in your stylesheets. If it's not defined, it will cause an error during the SCSS compilation.

src/styles/buy.scss
font-size: 14px;
line-height: 16px;
border-radius: $rounding;
transition: $transition-duration;
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 $transition-duration is defined in your stylesheets. If it's not defined, it will cause an error during the SCSS compilation.

src/styles/buy.scss
transition: $transition-duration;

&:hover {
background-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 $white is defined in your stylesheets. If it's not defined, it will cause an error during the SCSS compilation.

src/styles/nav.scss
}

&--active {
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 $blue-accent is defined in your stylesheets. If it's not defined, it will cause an error during the SCSS compilation.

src/styles/nav.scss
width: 100%;
height: 4px;
border-radius: 8px;
background-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 $blue-accent is defined in your stylesheets. If it's not defined, it will cause an error during the SCSS compilation.

src/styles/page.scss
font-weight: 400;
font-size: 12px;
line-height: 14px;
color: $main-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 $main-accent is defined in your stylesheets. If it's not defined, it will cause an error during the SCSS compilation.

src/styles/stars.scss
}

&--active :not(:last-child) {
background-image: url(/src/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 star-active.svg exists at the path /src/images/. If it's not present, the active star background will not display correctly.

src/styles/stars.scss
&__item {
width: 16px;
height: 16px;
background-image: url(/src/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 star.svg exists at the path /src/images/. If it's not present, the star background will not display correctly.

mateacademy-ai-mentor
mateacademy-ai-mentor approved these changes Nov 9, 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! 🎉 Your code is mostly well-structured and follows the task requirements. However, there are a few critical issues that need attention:

  1. Grid Layout: The current implementation of grid-template-columns does not meet the task requirement of displaying 1, 2, 3, or 4 columns based on screen resolution. Please adjust the grid settings to ensure it adapts to the specified breakpoints: 1 column for smaller screens, 2 starting at 488px, 3 starting from 768px, and 4 starting from 1024px.

  2. data-qa Attributes: Ensure all interactive elements, such as the 'Buy' button, have the required data-qa attributes for testing purposes. This is crucial for the testing and validation process.

Despite these issues, the decision to approve your pull request is made to help you move forward with your learning. Please review the comments and address these points to improve your code. 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

<a
href="#"
class="buy"
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 is missing for this 'Buy' button. Ensure all interactive elements have the required data-qa attributes for testing purposes.

src/index.html

<a
href="#"
class="buy"
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 is missing for this 'Buy' button. Ensure all interactive elements have the required data-qa attributes for testing purposes.

src/index.html

<a
href="#"
class="buy"
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 is missing for this 'Buy' button. Ensure all interactive elements have the required data-qa attributes for testing purposes.

src/index.html

<a
href="#"
class="buy"
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 is missing for this 'Buy' button. Ensure all interactive elements have the required data-qa attributes for testing purposes.

src/index.html

<a
href="#"
class="buy"
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 is missing for this 'Buy' button. Ensure all interactive elements have the required data-qa attributes for testing purposes.

src/styles/card.scss
}

&:hover .card__title {
color: $title-hover;
Copy link

Choose a reason for hiding this comment

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

The variable $title-hover is used here and is correctly defined in variables.scss as #34568b.

src/styles/card.scss

&__code {
font-size: 10px;
color: $secondary;
Copy link

Choose a reason for hiding this comment

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

The variable $secondary is used here and is correctly defined in variables.scss as rgba(97, 96, 112, 1).

src/styles/card.scss

&__review {
font-size: 10px;
color: $main-accent;
Copy link

Choose a reason for hiding this comment

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

The variable $main-accent is used here and is correctly defined in variables.scss as rgba(6, 11, 53, 1).

src/styles/card.scss

&__price-text {
line-height: 18px;
color: $secondary;
Copy link

Choose a reason for hiding this comment

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

The variable $secondary is used here and is correctly defined in variables.scss as rgba(97, 96, 112, 1).

src/styles/card.scss
font-weight: 700;
font-size: 16px;
line-height: 18px;
color: $main-accent;
Copy link

Choose a reason for hiding this comment

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

The variable $main-accent is used here and is correctly defined in variables.scss as rgba(6, 11, 53, 1).

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