Design comparison
Solution retrospective
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
Community feedback
- @anisbenyPosted 7 months ago
Thank you for your feedback, I really appreciate your recommendations.
0 - @dvdgdPosted 7 months ago
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 apicture
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 thedisplay
property tocontents
: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 yourclamp(350px, 75%, 750px)
. Here's how to address this:- Remove the
clamp
for the min-width of thearticle
. - Add a
max-width
of350px
for the card on devices with screens smaller than or equal to768px
. 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.
- Remove the
-
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 be600px
, and for mobile devices350px
. 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!
0 -
- @homciAbdallahPosted 7 months ago
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
0
Please log in to post a comment
Log in with GitHubJoin our Discord community
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