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

moyo done #4836

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

moyo done #4836

wants to merge 2 commits into from

Conversation

mtereshyn
Copy link

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

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 and effort on this pull request! 🎉 While your implementation meets many of the task requirements, there are a few critical issues that need to be addressed before we can approve it:

  1. Font-Style Property: The font-style is incorrectly set to 400, which should be a font-weight value. Please correct this to font-style: normal; or italic as intended.

  2. Selector Specificity: Improve the specificity of selectors by using more descriptive naming conventions, such as stars--rating-1 instead of stars--1, to enhance readability and maintainability.

  3. Responsive Design: The padding values in the header and other components might not be responsive on smaller screens. Consider using relative units or media queries to ensure a consistent layout across different devices.

  4. Transition Property: Ensure the transition property applies to all state changes by moving transition: all 0.3s; to the main block instead of within pseudo-classes.

  5. Hover Effects: The hover effect on the card title should ensure sufficient contrast for readability, and the transform scale of 1.2 might be too large, potentially causing layout issues. Consider reducing the scale value.

  6. Code Optimization: The repeated product cards can be optimized using a templating system or JavaScript for dynamic generation, improving maintainability.

Please address these issues, and feel free to reach out if you have any questions or need further assistance. Keep up the great work, and I'm confident you'll nail it with these adjustments! 🚀

✨ 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 +156 to +427
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
</div>

<p class="card__rating-review">Reviews: 5</p>
</div>

<div class="card__price">
<p class="card__price-text">Price:</p>

<p class="card__price-value">$2,199</p>
</div>

<a
href="#"
class="card__button"
>
buy
</a>
</div>

<div class="card">
<img
src="./images/imac.jpeg"
class="card__image"
alt="APPLE A1419 iMac 27"
/>

<h3 class="card__title">
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</h3>

<p class="card__product-code">Product code: 195434</p>

<div class="card__rating">
<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__rating-review">Reviews: 5</p>
</div>

<div class="card__price">
<p class="card__price-text">Price:</p>

<p class="card__price-value">$2,199</p>
</div>

<a
href="#"
class="card__button"
>
buy
</a>
</div>

<div class="card">
<img
src="./images/imac.jpeg"
class="card__image"
alt="APPLE A1419 iMac 27"
/>

<h3 class="card__title">
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</h3>

<p class="card__product-code">Product code: 195434</p>

<div class="card__rating">
<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__rating-review">Reviews: 5</p>
</div>

<div class="card__price">
<p class="card__price-text">Price:</p>

<p class="card__price-value">$2,199</p>
</div>

<a
href="#"
class="card__button"
>
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 multiple times with identical content. This repetition can be optimized by using a templating system or JavaScript to dynamically generate the cards. This approach will make the code more maintainable and easier to update.

src/styles/blocks/card.scss Outdated
}

&:hover {
transform: scale(1.2);
Copy link

Choose a reason for hiding this comment

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

The transform scale of 1.2 on hover might be too large and could cause layout issues or overlap with other elements. Consider using a smaller scale value to ensure the card remains within its intended space.

src/styles/blocks/card.scss Outdated
Comment on lines 89 to 91
&:hover &__title {
color: $color-steel-blue;
}
Copy link

Choose a reason for hiding this comment

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

The hover effect on the card title changes its color to $color-steel-blue. Ensure this color provides sufficient contrast against the background for readability.

src/styles/blocks/header.scss Outdated
display: flex;
justify-content: space-between;
align-items: center;
padding: 0 50px;
Copy link

Choose a reason for hiding this comment

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

The padding of 0 50px on the header might not be responsive on smaller screens. Consider using relative units or media queries to adjust the padding for different screen sizes.

src/styles/blocks/header.scss

&:hover {
color: $color-vivid-cerulean;
transition: all 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 should be placed outside of the pseudo-class to ensure it applies to all state changes, not just the hover state. Consider moving transition: all 0.3s; to the &__link block.

src/styles/blocks/main.scss
@@ -0,0 +1,8 @@
.container {
display: grid;
padding: 50px 40px;
Copy link

Choose a reason for hiding this comment

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

The padding of 50px 40px might not be responsive on smaller screens. Consider using relative units or media queries to adjust the padding for different screen sizes to ensure a consistent layout.

src/styles/blocks/stars.scss Outdated
Comment on lines 16 to 20
&--1 &__star:nth-child(-n + 1),
&--2 &__star:nth-child(-n + 2),
&--3 &__star:nth-child(-n + 3),
&--4 &__star:nth-child(-n + 4),
&--5 &__star:nth-child(-n + 5) {
Copy link

Choose a reason for hiding this comment

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

The specificity of these selectors could be improved by using a more descriptive naming convention for the modifiers (e.g., stars--rating-1 instead of stars--1). This would make the code more readable and maintainable, especially if more modifiers are added in the future.

src/styles/utils/global.scss Outdated
@@ -0,0 +1,16 @@
html {
font-family: Roboto, sans-serif;
font-style: 400;
Copy link

Choose a reason for hiding this comment

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

The font-style property is incorrectly set to 400, which is a font-weight value. It should be set to normal or italic. Consider changing it to font-style: normal; if the intention is to use the normal style.

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.

Great job on your submission! 🎉 Your code is well-structured and demonstrates a good understanding of HTML and SCSS. However, there are a few areas that could use improvement:

  • Repeated Product Cards: The product cards are currently hardcoded with identical content. To enhance maintainability, consider using a templating system or JavaScript for dynamic generation.

  • Selector Specificity: Improve the readability and maintainability of your code by using more descriptive class names, such as stars--rating-4 instead of stars--4.

  • Accessibility: Ensure that the alt attributes for images, especially the logo, are descriptive enough for users relying on screen readers.

  • Responsive Design: Use relative units or media queries for padding values to ensure a consistent layout across different devices. This applies to both the main container and the header component.

  • Transition Property: Consider moving the transition property to the main .nav__link block to cover all state changes, not just the :hover state.

  • Hover Effects: The transform scale of 1.1 might be too large, potentially causing layout issues. Consider reducing the scale value to maintain visual integrity.

These adjustments will not only improve the functionality and accessibility of your code but also enhance its maintainability. Keep up the great work, and continue to learn from these insights to refine your skills! 🚀

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

src/index.html
Comment on lines +156 to +427
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
</div>

<p class="card__rating-review">Reviews: 5</p>
</div>

<div class="card__price">
<p class="card__price-text">Price:</p>

<p class="card__price-value">$2,199</p>
</div>

<a
href="#"
class="card__button"
>
buy
</a>
</div>

<div class="card">
<img
src="./images/imac.jpeg"
class="card__image"
alt="APPLE A1419 iMac 27"
/>

<h3 class="card__title">
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</h3>

<p class="card__product-code">Product code: 195434</p>

<div class="card__rating">
<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__rating-review">Reviews: 5</p>
</div>

<div class="card__price">
<p class="card__price-text">Price:</p>

<p class="card__price-value">$2,199</p>
</div>

<a
href="#"
class="card__button"
>
buy
</a>
</div>

<div class="card">
<img
src="./images/imac.jpeg"
class="card__image"
alt="APPLE A1419 iMac 27"
/>

<h3 class="card__title">
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</h3>

<p class="card__product-code">Product code: 195434</p>

<div class="card__rating">
<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__rating-review">Reviews: 5</p>
</div>

<div class="card__price">
<p class="card__price-text">Price:</p>

<p class="card__price-value">$2,199</p>
</div>

<a
href="#"
class="card__button"
>
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 system or JavaScript to dynamically generate these cards to improve maintainability.

src/index.html
<p class="card__product-code">Product code: 195434</p>

<div class="card__rating">
<div class="stars stars--4">
Copy link

Choose a reason for hiding this comment

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

The class name stars--4 could be more descriptive to enhance readability and maintainability. Consider using a naming convention that reflects the rating value, such as stars--rating-4.

src/index.html
Comment on lines +27 to +30
<img
src="./images/logo.png"
alt="moyo logo"
class="header__logo"
Copy link

Choose a reason for hiding this comment

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

Ensure the alt attribute for the logo image is descriptive enough to convey the image content to users who rely on screen readers.

src/styles/blocks/card.scss
}

&:hover {
transform: scale(1.1);
Copy link

Choose a reason for hiding this comment

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

The transform scale of 1.1 might be too large, potentially causing layout issues. Consider reducing the scale value to maintain the visual integrity of the layout.

src/styles/blocks/header.scss
Comment on lines +5 to +8
padding: 0 5vw;

@media (min-width: 768px) {
padding: 0 50px;
Copy link

Choose a reason for hiding this comment

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

The padding values use vw units for smaller screens and fixed px values for larger screens. Consider using relative units or additional media queries to ensure a consistent layout across different devices.

src/styles/blocks/header.scss

&:hover {
color: $color-vivid-cerulean;
transition: all 0.3s;
Copy link

Choose a reason for hiding this comment

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

Consider moving the transition property to the main .nav__link block to ensure it covers all state changes, not just the :hover state.

src/styles/blocks/main.scss
@@ -0,0 +1,8 @@
.container {
display: grid;
padding: 50px 40px;
Copy link

Choose a reason for hiding this comment

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

The padding values are fixed, which might not be responsive on smaller screens. Consider using relative units or media queries to ensure a consistent layout across different devices.

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