add task solution #4808
add task solution #4808
Conversation
JPodgrodzki
commented
Oct 26, 2024
JPodgrodzki
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
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 data-qa attributes need correction, specifically changing data-qa="hover" to data-qa="card-hover". Additionally, the SCSS file is missing essential media queries for the grid layout and hover effects that are part of the core requirements. Please make these adjustments to ensure the functionality aligns with the task specifications. 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
| <ul class="nav__list"> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="index.html" |
There was a problem hiding this comment.
The href attribute for this link points to index.html. If this is meant to navigate to a different section or page, ensure the correct URL is used.
src/index.html
Outdated
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="index.html" |
There was a problem hiding this comment.
The href attribute for this link points to index.html. If this is meant to navigate to a different section or page, ensure the correct URL is used.
src/index.html
Outdated
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="index.html" |
There was a problem hiding this comment.
The href attribute for this link points to index.html. If this is meant to navigate to a different section or page, ensure the correct URL is used.
src/index.html
Outdated
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="index.html" |
There was a problem hiding this comment.
The href attribute for this link points to index.html. If this is meant to navigate to a different section or page, ensure the correct URL is used.
src/index.html
Outdated
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="index.html" |
There was a problem hiding this comment.
The href attribute for this link points to index.html. If this is meant to navigate to a different section or page, ensure the correct URL is used.
src/index.html
Outdated
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="index.html" |
There was a problem hiding this comment.
The href attribute for this link points to index.html. If this is meant to navigate to a different section or page, ensure the correct URL is used.
src/index.html
Outdated
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="index.html" |
There was a problem hiding this comment.
The href attribute for this link points to index.html. If this is meant to navigate to a different section or page, ensure the correct URL is used.
src/index.html
Outdated
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="index.html" |
There was a problem hiding this comment.
The href attribute for this link points to index.html. If this is meant to navigate to a different section or page, ensure the correct URL is used.
src/styles/index.scss
Outdated
| &:hover &__name { | ||
| color: #34568b; | ||
| } |
There was a problem hiding this comment.
The &:hover &__name selector seems misplaced. It targets .card__name when .card__price is hovered, which might not be the intended behavior. Ensure this is correct or adjust the selector to target the intended element.
src/index.html
Outdated
| <li class="nav__item"> | ||
| <a | ||
| href="#" | ||
| class="nav__link nav__link--hover" |
There was a problem hiding this comment.
Co robi ta klasa --hover i czemu inne elementy jej nie maja? Pewnie zapomniałes usunac czy cos
src/styles/index.scss
Outdated
| transition: transform 300ms; | ||
|
|
||
| &__image { | ||
| background-image: url(../images/imac.jpeg); |
There was a problem hiding this comment.
Nie jestem fanem dawanie tak waznych grafik jak grafiki produktów na background-image.
Pamietaj ze strona ładuje sie w tej kolejnosci HTML > CSS > JS wiec wkładajac tak wazne grafiki do CSS opozniasz ich wywołanie.
No jest to tez gorsze rozwiazanie pod SEO ponieważ roboty przegladajace twoja strone nie widza tych grafik. jak mozesz zauważyć nawet nie mozesz im dodać atrybutu ALT
src/styles/index.scss
Outdated
| &-star { | ||
| height: 16px; | ||
| width: 16px; | ||
| background-image: url(../images/star.svg); |
There was a problem hiding this comment.
Gwiazdki juz raczej moge byc na background-image tutaj to jest chyba bez znaczenia
src/index.html
Outdated
| <li class="nav__item"> | ||
| <a | ||
| href="#" | ||
| class="nav__link nav__link--active is-active" |
There was a problem hiding this comment.
you don't need nav__link--active modifier because you can use is-active instead
src/styles/index.scss
Outdated
| font-family: Roboto, sans-serif; | ||
| } | ||
|
|
||
| .header { |
There was a problem hiding this comment.
create separate files for all of BEM blocks (header, nav, container, card, stars)
src/index.html
Outdated
| <div class="card__image"> | ||
| <img | ||
| src="./images/imac.jpeg" | ||
| class="photo card__image--photo" |
There was a problem hiding this comment.
A modifier must not be used without the class it modifies
| class="photo card__image--photo" | |
| class="card__image-photo" |
src/index.html
Outdated
| <li class="nav__item"> | ||
| <a | ||
| href="#" | ||
| class="nav__link nav__link--active is-active" |
There was a problem hiding this comment.
| class="nav__link nav__link--active is-active" | |
| class="nav__link is-active" |
| class="card__buy" | ||
| data-qa="card-hover" | ||
| > | ||
| <span class="card__buy-text">BUY</span> |
There was a problem hiding this comment.
you don't use card__buy-text class and you don't need span inside button
src/styles/blocks/card.scss
Outdated
| &:hover &__name { | ||
| color: #34568b; | ||
| } |
There was a problem hiding this comment.
this selector is incorrect but correct version is located below &:hover .card__name
