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.

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

add task solution #4826

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

AlinaYamchuk
Copy link

  • All component follow BEM and use SCSS

  • repaeted 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 ❗️]

  • DEMO LINK

  • TEST REPORT LINK

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. The SCSS file needs to be compiled into CSS, as browsers cannot interpret SCSS directly. Additionally, there are inconsistencies in class naming for active navigation links, which could lead to styling issues. Please ensure all necessary variables and mixins are defined to avoid compilation errors. 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.

@@ -17,11 +17,696 @@
/>
<link
rel="stylesheet"
href="styles/index.scss"
href="/src/styles/index.scss"

Choose a reason for hiding this comment

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

The stylesheet link references an SCSS file (index.scss). Browsers cannot directly interpret SCSS files; they need to be compiled into CSS. Ensure that the SCSS is compiled to CSS and the link is updated to point to the resulting CSS file.

Comment on lines +42 to +69
class="nav__link is-active"
>
Apple
</a>
</li>

<li class="nav__item">
<a
href="#"
class="nav__link"
>
Samsung
</a>
</li>

<li class="nav__item">
<a
href="#"
class="nav__link"
>
Smartphones
</a>
</li>

<li class="nav__item">
<a
href="#"
class="nav__link active"

Choose a reason for hiding this comment

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

There is an inconsistency in the class naming for active navigation links. The first link uses 'is-active' while the fourth link uses 'active'. It's important to maintain consistency in class naming conventions to avoid confusion and potential styling issues.

font-size: 10px;
font-weight: 400;
font-style: normal;
color: $main-text-color;

Choose a reason for hiding this comment

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

The variable $main-text-color is used here, but it's important to ensure that it is defined in the imported variables file. Otherwise, it will cause an error during compilation.

color: $main-text-color;

@include max-width;
@include min-width;

Choose a reason for hiding this comment

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

The mixins max-width and min-width are used here. Ensure these mixins are defined in the imported mixin file, otherwise, it will lead to a compilation error.

Comment on lines +77 to +78
.nav__link.active,
.nav__link.is-active {

Choose a reason for hiding this comment

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

There is an inconsistency in the class naming for active navigation links. Both 'active' and 'is-active' are used. It's important to maintain consistency in class naming conventions to avoid confusion and potential styling issues.

&--2 &__star:nth-child(n + 3),
&--3 &__star:nth-child(n + 4),
&--4 &__star:nth-child(n + 5) {
background-image: $star-bg;

Choose a reason for hiding this comment

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

The variable $star-bg is used here for the background image. Ensure that this variable is defined in the appropriate SCSS file to avoid compilation errors.

&--3 &__star:nth-child(-n + 3),
&--4 &__star:nth-child(-n + 4),
&--5 &__star:nth-child(-n + 5) {
background-image: $star-bg-active;

Choose a reason for hiding this comment

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

The variable $star-bg-active is used here for the background image. Ensure that this variable is defined in the appropriate SCSS file to avoid compilation errors.

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 submission! 🎉 Your code demonstrates a solid understanding of HTML structure and SCSS organization. However, there are a few areas that need attention to fully meet the task requirements:

  1. Grid Usage for Columns: Ensure the grid layout adapts to different screen sizes as specified, using media queries to adjust the number of columns.

  2. Card Hover Effects: Implement smooth hover effects over 300ms, including increasing card size by 20% and changing the card title color to #34568b.

  3. Data-qa Attributes: Maintain consistent data-qa attribute values across similar elements for better uniformity.

  4. Card Container Padding and Card Gap: Adjust the card container padding and gaps to match the specified dimensions.

  5. Variable Usage: Ensure all repeated sizes and special colors are defined as variables for better maintainability.

  6. Class Naming Consistency: Use consistent class names for active states in navigation links.

These improvements will enhance the functionality and maintainability of your code. Keep up the great work, and use this feedback to refine your skills! 🚀

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

<li class="nav__item">
<a
href="#"
class="nav__link is-active"

Choose a reason for hiding this comment

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

The class 'is-active' is used here, but on line 69, the class 'active' is used. Ensure consistency in class naming for active states across your navigation links.

<li class="nav__item">
<a
href="#"
class="nav__link active"

Choose a reason for hiding this comment

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

The class 'active' is used here, but on line 42, the class 'is-active' is used. Ensure consistency in class naming for active states across your navigation links.

</p>
<button
class="card__buy-button"
data-qa="card-hover"

Choose a reason for hiding this comment

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

The 'data-qa' attribute value 'card-hover' is used here, but on line 262, the value 'hover' is used. Consider using consistent naming for 'data-qa' attributes across similar elements.

</p>
<button
class="card__buy-button"
data-qa="hover"

Choose a reason for hiding this comment

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

The 'data-qa' attribute value 'hover' is used here, but on line 188, the value 'card-hover' is used. Consider using consistent naming for 'data-qa' attributes across similar elements.

Comment on lines +77 to +78
.nav__link.active,
.nav__link.is-active {

Choose a reason for hiding this comment

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

The use of both 'active' and 'is-active' classes for navigation links is inconsistent. Consider choosing one naming convention for active states to maintain consistency across your styles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants