@anisbeny
Posted
Thank you for your feedback, I really appreciate your recommendations.
What are you most proud of, and what would you do differently next time?
I'm proud of the result
What challenges did you encounter, and how did you overcome them?
nothing to report
What specific areas of your project would you like help with?
nothing to report
@anisbeny
Posted
Thank you for your feedback, I really appreciate your recommendations.
@dvdgd
Posted
Hello! You've done a great job—it's nearly perfect.
I just have a few points to cover to help you improve it even further:
While viewing on a mobile screen, I noticed that the image source doesn't change. You can fix this by replacing the figure
with a picture
and adding different sources with breakpoints. Here's an example:
<picture>
<source media="(min-width: 768px)" srcset="./images/image-product-desktop.jpg">
<img src="./images/image-product-mobile.jpg" alt="Perfume bottle">
</picture>
When using picture
, ensure to set the display
property to contents
: display: contents
.
By default, the <picture>
element behaves like a block-level element, meaning it takes up the full width of its parent container. However, the <img>
element inside it behaves like an inline-level element. This means that any padding, margins, or borders applied to the <picture> element will affect its layout, potentially causing unintended spacing or alignment issues.
Setting the display property of the <picture>
element to contents alters its behavior. The contents value causes the <picture>
element to behave as if it were not there in terms of layout, essentially making its children (in this case, the <source>
and <img>
elements) direct children of the <picture>
element's parent. This ensures that any styles applied to the <picture>
element itself do not interfere with the layout of its children.
I noticed that the card grows on mobile but doesn't shrink less than 350px
due to your clamp(350px, 75%, 750px)
. Here's how to address this:
clamp
for the min-width of the article
.max-width
of 350px
for the card on devices with screens smaller than or equal to 768px
. This way, you'll only need to consider two variations: desktop and mobile. This ensures the card doesn't grow excessively but can still shrink for very small screens.For your price, you could add flex-wrap: wrap
. This will prevent the price from overflowing on small screens.
TIP: For desktop, the max-width
of the card could be 600px
, and for mobile devices 350px
. This could simplify centering the card using flex-box.
PLUS: There are several ways to center an element on the page. But instead of using position and transform, you could utilize flex-box, where it remains a cleaner way and helps us deal with responsiveness, and I saw you already know how it works. Here's an example::
body {
display: flex;
justify-content: center;
align-items: center;
min-height: 100svh; /* Ensure your page occupies all available height */
}
I hope this feedback helps you improve your solution. Feel free to ask anything, and if you'd like, I can create a PR on GitHub to assist you further!
I appreciate how you've made your HTML semantic and opted for CSS selectors instead of creating classes—awesome!
Keep up the good work and happy coding!
@homciAbdallah
Posted
Does the solution include semantic HTML? Yes in deed Is it accessible, and what improvements can be made? Can and improvements in size Does the design look good on a range of screen sizes? Yes Is the code well structured, readable, and reusable? Yes Is the solution significantly different from the design? a little
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord