-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Solution #4984
base: master
Are you sure you want to change the base?
Solution #4984
Conversation
There was a problem hiding this 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! 😊 While most of your code is well-structured and meets the task requirements, there's a critical issue with the button hover effect that needs to be addressed. The task specifies a specific hover effect for buttons that is not currently implemented correctly. Please adjust the button hover effect to match the task requirements, and you'll be all set! 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
Outdated
<a | ||
href="index.html" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The href
attribute for the navigation link is pointing to index.html
, which is the same page. If this is intentional, it's fine, but if these links are supposed to navigate to different pages, ensure they are pointing to the correct URLs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, the links don't lead anywhere. Is it better to leave index.html or #?
src/index.html
Outdated
<a | ||
href="index.html" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the previous comment, ensure that the href
attribute is pointing to the correct destination if this link is supposed to navigate to a different page.
src/index.html
Outdated
<a | ||
href="index.html" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the href
attribute is pointing to the correct destination if this link is supposed to navigate to a different page.
src/index.html
Outdated
<a | ||
href="index.html" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the href
attribute is pointing to the correct destination if this link is supposed to navigate to a different page.
src/index.html
Outdated
<a | ||
href="index.html" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the href
attribute is pointing to the correct destination if this link is supposed to navigate to a different page.
<a | ||
href="#" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The href
attribute is set to #
, which means it doesn't lead anywhere. If this is a placeholder, it's fine, but ensure to update it with a valid URL if needed.
<a | ||
href="#" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The href
attribute is set to #
, which means it doesn't lead anywhere. If this is a placeholder, it's fine, but ensure to update it with a valid URL if needed.
<a | ||
href="#" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The href
attribute is set to #
, which means it doesn't lead anywhere. If this is a placeholder, it's fine, but ensure to update it with a valid URL if needed.
<a | ||
href="#" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The href
attribute is set to #
, which means it doesn't lead anywhere. If this is a placeholder, it's fine, but ensure to update it with a valid URL if needed.
<a | ||
href="#" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The href
attribute is set to #
, which means it doesn't lead anywhere. If this is a placeholder, it's fine, but ensure to update it with a valid URL if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, the links don't lead anywhere. Is it better to leave index.html or #?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on implementing the task requirements! 🎉 Your code is well-structured, and you've effectively used SCSS for styling. However, there are a few areas to improve:
-
Button Hover Effect: Ensure the button hover effect changes the background to
#fff
and the text color to#00acdc
as per the task requirements. The current implementation with a transparent background and border doesn't align with these requirements. -
Product Card Content: The product cards currently display identical content. If the goal is to showcase different products, consider using dynamic content generation to populate these cards with unique data.
-
Button Height on Hover: The hover effect reduces the button height by 1px, which might cause a visual jump. Keeping the height consistent will ensure a smoother transition.
These adjustments will enhance the functionality and user experience of your project. Keep up the great work, and continue learning from these insights to refine your skills! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
<img | ||
src="./images/imac.jpeg" | ||
alt="APPLE A1419 iMac 27'' Retina" | ||
class="card__image" | ||
/> | ||
<div class="card__content"> | ||
<h3 class="card__title"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h3> | ||
<p class="card__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> | ||
<span class="card__reviews">Reviews: 5</span> | ||
</div> | ||
<div class="card__price"> | ||
Price: | ||
<span class="card__price-value">$2,199</span> | ||
</div> | ||
<a | ||
href="#" | ||
class="card__button" | ||
> | ||
BUY | ||
</a> | ||
</div> | ||
</div> | ||
|
||
<div class="card"> | ||
<img | ||
src="./images/imac.jpeg" | ||
alt="APPLE A1419 iMac 27'' Retina" | ||
class="card__image" | ||
/> | ||
<div class="card__content"> | ||
<h3 class="card__title"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h3> | ||
<p class="card__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> | ||
<span class="card__reviews">Reviews: 5</span> | ||
</div> | ||
<div class="card__price"> | ||
Price: | ||
<span class="card__price-value">$2,199</span> | ||
</div> | ||
<a | ||
href="#" | ||
class="card__button" | ||
> | ||
BUY | ||
</a> | ||
</div> | ||
</div> | ||
|
||
<div class="card"> | ||
<img | ||
src="./images/imac.jpeg" | ||
alt="APPLE A1419 iMac 27'' Retina" | ||
class="card__image" | ||
/> | ||
<div class="card__content"> | ||
<h3 class="card__title"> | ||
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A) | ||
</h3> | ||
<p class="card__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> | ||
<span class="card__reviews">Reviews: 5</span> | ||
</div> | ||
<div class="card__price"> | ||
Price: | ||
<span class="card__price-value">$2,199</span> | ||
</div> | ||
<a | ||
href="#" | ||
class="card__button" | ||
> | ||
BUY | ||
</a> | ||
</div> | ||
</div> |
There was a problem hiding this comment.
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. If the intention is to display different products, consider using a dynamic approach to populate these cards with varying data. This will enhance maintainability and scalability.
&:hover { | ||
background-color: transparent; | ||
color: $primary-color; | ||
border: 1px solid $primary-color; | ||
height: 23px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hover effect for the button reduces the height by 1px (from 24px to 23px). This might not be necessary and could lead to a slight visual jump. Consider keeping the height consistent during hover to maintain a smooth transition.
:hover
are smooth