Responsive product preview card

Community feedback

• 

  Kristian Kalvå•360

  @kkalvaaPosted over 2 years ago

  A couple notes:

  1: I find it odd that you're using two images for the image, one for desktop and one for mobile. The way you've implemented it, the browser will load both and "hide" one. If the goal is to make it faster on mobile you've actually made it worse by forcing the browser to load both, but not show the large one. It would be better if you just loaded the desktop version on scaled it down.

  I guess the point is to save bandwidth and only serve the current viewport image. If that really is the case you should be using srcset.

  2: This might be just me, but I find it very odd that you're writing attributes with a space around the =. 'alt = "some text"' instead of 'alt="some text"'. I guess it is valid with spaces, but it strikes me as odd and it is the first time I've ever seen it written that way.

  3: While not too important on this reduced task it is worth taking into account. CSS imports are inefficient and you should import your fonts in HTML instead of in CSS. (sadly I forgot my source)

  4: :root and * should come first in your CSS, not after .attribution and .attribution a. That way the least specific things comes first in your cascade and you can overwrite styles with more spesific things later on.

  5: I find the responsiveness of your component to be very jittery. Sometimes it's large and sometimes it is small. I think this is because you specifically set widths and heights for various screen sizes. Ideally you should abuse "max-width: 760px" (or some other width) and not set height at all. only change the max-width when things break down completely.

  6: On ".content-container h3" (and many others) you're setting "padding: 1.5em 0em 0em 1.5em;". It is considered best practice to be specific and instead go "padding-left: 1.5em; padding-top: 1.5em;" instead. However, you shouldn't really set padding on the items, but the padding should be on ".content-container" instead. If you set the padding on the container you're going to save yourself a big headache by having to only manage padding once instead of on every single child element.

  7: I would recommend looking into BEM for naming things in css.

  Marked as helpful

  1

  ---

  

  shemjay•140

  @shemjayPosted over 2 years ago

  @kkalvaa Hey thanks for the in-depth reply. I knewmy code had a lot of issues so i'll redo it with this new helpful information. Just a few replies to some things you pointed out.

  1. I mostly used two images because I havent yet gotten the hang of using srcset. When I tried to use the images were not changing at the specific breakpoints to ill need to work on that.

  2. Lol this is a habit I picked up from a computer science teacher in school. Guess I never noticed. Sometimes I put a space in between the alt and text while other times I don't. Guess I should start going with the status quo.

  3. I'm guessing this was your source? https://www.stevesouders.com/blog/2009/04/09/dont-use-import/#comments Even if wasn't I think from now on i will just use link instead of import.

  4. Thanks I often start writing code underneath the .attribution cause it comes with the template then I forget to remove it. My bad.

  5. Yeah I noticed that too i knew it was a bad idea but I wasnt sure how else to implement it. Ill make use of max-width now.

  6. Applying padding to each element was a bad idea. It made the process of applying breakpoints a nightmare lol. Noted ill do that from now on.

  Thanks again for all the tips!

  0