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

Closed
wants to merge 4 commits into from
Closed

add task solution #4667

wants to merge 4 commits into from

Conversation

Menwell
Copy link

@Menwell Menwell commented Sep 13, 2024

❗️ Copy this Checklist to the Pull Request description after links, and put - [x] before each point after you checked it.

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

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 effort on the structure and organization of your SCSS files! 👏 However, before approving, we need to address a few critical issues to ensure the functionality and maintainability of your code:

  1. Update the paths used for linking resources and images to be relative and correctly enclosed in quotes. This will ensure that your resources load correctly on all platforms.
  2. Implement the DRY principle to avoid code repetition, which will make your codebase more maintainable in the long run.
  3. Correct the use of CSS properties, such as align-items and transform, to ensure that your styles work as intended across all browsers.
  4. Refactor code to use variables for repetitive values and ensure adherence to the code style rules, like using kebab-case for variable names and including a generic font family as a fallback.

Once these changes are made, your code will be in great shape! Keep up the good work, and remember to test your paths and styles across different browsers to catch any compatibility issues. 🛠️💻

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

src/index.html Outdated
class="header__logo"
>
<img
src="/src/images/logo.png"

Choose a reason for hiding this comment

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

Avoid using absolute paths for images, use relative paths instead. This ensures that the image can be accessed regardless of the domain or directory structure.

class="card"
>
<img
src="./images/imac.jpeg"

Choose a reason for hiding this comment

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

The src attribute should use a relative path, not a path that looks like it's absolute. Use './images/imac.jpeg' instead of '/src/images/imac.jpeg'.

src/index.html Outdated

<div class="card">
<img
src="/src/images/imac.jpeg"

Choose a reason for hiding this comment

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

The src attribute should use a relative path, not a path that looks like it's absolute. Use './images/imac.jpeg' instead of '/src/images/imac.jpeg'.

Comment on lines +110 to +147
<div
data-qa="card"
class="card"
>
<img
src="./images/imac.jpeg"
alt="product"
class="card__image"
/>

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

<p class="card__code">Product code: 195434</p>

<div class="card__rates">
<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__rates-reviews">Reviews: 5</p>
</div>

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

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

<button
class="card__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.

Repeated code: The card component is repeated multiple times with the same content. Consider using a loop to generate these cards if you are using a templating engine or a front-end framework. If you are not using a templating engine, at least ensure that the content is unique for each card if they are meant to be different.

border: 1px solid $main-card-border-color;
border-radius: 5px;
padding: 32px 16px 16px;
transition: scale 0.3s;

Choose a reason for hiding this comment

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

The CSS property for transitions should be transform instead of scale. Replace scale with transform to fix the transition effect.

@@ -0,0 +1,10 @@
html {
font-family: Roboto, serif;

Choose a reason for hiding this comment

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

According to the Code Style Rules ❗️, font-families should be listed with fallbacks. It's a good practice to include a generic font family as a fallback. In this case, 'sans-serif' should be used instead of 'serif' as 'Roboto' is a sans-serif font.

Comment on lines 5 to 9
@for $i from 1 through 5 {
&--#{$i} &__star:nth-child(-n + #{$i}) {
background-image: url(../images/star-active.svg);
}
}

Choose a reason for hiding this comment

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

According to the Code Style Rules ❗️, when using SCSS loops, it's recommended to include a newline after the opening brace { and before the closing brace } to improve readability.

}

&__star {
background-image: url(../images/star.svg);

Choose a reason for hiding this comment

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

The paths to the images should be enclosed in quotes according to the code style rules.


@for $i from 1 through 5 {
&--#{$i} &__star:nth-child(-n + #{$i}) {
background-image: url(../images/star-active.svg);

Choose a reason for hiding this comment

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

The paths to the images should be enclosed in quotes according to the code style rules.

Comment on lines 15 to 16
width: 16px;
height: 16px;

Choose a reason for hiding this comment

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

It's better to store repetitive values such as the width and height of the stars in variables for easier maintenance and to avoid magic numbers. Consider adding them to src/styles/utils/variables.scss.

@Menwell
Copy link
Author

Menwell commented Sep 13, 2024

Снимок экрана 2024-09-13 в 02 50 17 here in the task deploy doesn't work here is a screenshot with all the tests, I completed the task a week ago, tried everything, wrote to mentors, nothing helps, here is a screenshot

Copy link

@volodymyr-soltys97 volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

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

Hi, your demo link isn't working, you need to make a deploy and check it after on the demo
image

src/index.html Outdated
Comment on lines 36 to 40
<nav class="header__navigation navigation">
<ul class="navigation__list">
<li class="navigation__item">
<a
class="navigation__link is-active"

Choose a reason for hiding this comment

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

Suggested change
<nav class="header__navigation navigation">
<ul class="navigation__list">
<li class="navigation__item">
<a
class="navigation__link is-active"
<nav class="nav">
<ul class="na__list">
<li class="nav__item">
<a
class="nav__link is-active"

src/index.html Outdated
Comment on lines 45 to 46
</li>
<li class="navigation__item">

Choose a reason for hiding this comment

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

Add the blank line between the links

Suggested change
</li>
<li class="navigation__item">
</li>
<li class="navigation__item">

transition: scale 0.3s;

&:hover {
scale: 1.2;

Choose a reason for hiding this comment

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

Fix it
image

font-size: 12px;
font-weight: 500;
line-height: 18px;
letter-spacing: 0%;

Choose a reason for hiding this comment

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

And here

Copy link

@anastasiiavorobiova anastasiiavorobiova left a comment

Choose a reason for hiding this comment

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

Great work! Consider deploying the page and fixing AI comments


.is-active::after {
content: '';
width: 37px;

Choose a reason for hiding this comment

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

Suggested change
width: 37px;
width: 100%;

@Menwell Menwell closed this by deleting the head repository Sep 14, 2024
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.

4 participants