@elaineleung
Posted
Hi Cas, well done on another solution! 😊 I'll try to answer some of your questions here:
-
I think the semantics look fine, and good use of the
picture
element! The way you've written it is slightly different than how MDN writes it (they have theimg
containing the image that's in your main code, which in your case should be the mobile version since you did this with a mobile first approach), but as long as the picture switches and everything works, then that's fine. -
I generally use
rem
to handle limiting the width and dealing with responsiveness. Here's something you can try: Instead of amax-width
, usewidth: min(100%, 30rem)
, and for the desktop view, change that towidth: min(100%, 60rem)
. I also suggest making the breakpoint higher, maybe at 980px, so that when the layout changes, the displayed part of the image can still show the important parts of the image (or alternatively, you can also position the image in a way to do show what you want to show). -
About the CSS, I think you did well using a mobile first approach; the only thing I'd probably do differently is to format the code (which can be done easily in VS Code) to get rid of some of the spacing, and I'd also use different class names for some of these selectors. For example, I would call the image container
card-image
and the text containercard-content
, just so it's clear that these two are children of the card.
Last note: I think using viewport widths and percentages for widths can work if you use something like min()
(as in, what I did here) where you have other options like a fixed relative width (e.g., 30rem). Using just viewport or percentage widths alone can lead to undesirable outcomes at times if there aren't other properties included.
Hope some of this feedback can help you out!
Marked as helpful