Responsive Product Card Preview with HTML and CSS

Community feedback

• 

  T

  Grace•31,090

  @grace-snowPosted 3 months ago

  This looks pretty good but there are some important issues to raise:

  1. All content should be contained within landmarks in HTML. This, and every page you do, needs the main content wrapping in a main landmark. The attribution should be in a footer.
  2. Never have text in divs or spans alone. Always use a meaningful element, even a paragraph. In this the old price needs to be wrapped in either a s (strikethrough) tag or a del (delete) tag as it is the old price. I've written up a detailed post about choosing appropriate html elements on my site FEDmentor.dev if you want to take a look.
  3. Don't add extra divs for no reason if you can avoid it. Try to keep html as simple as possible. The button, for example, doesn't need a wrapping div.
  4. The cart icon is decorative so alt must be empty. The main image is important so must have a more detailed description (it shouldn't just repeat the perfume name, it must communicate the same value/meaning that the image does.
  5. I strongly recommend against using the old "62.5% font size hack". You really don't need to do this and it can cause big accessibility problems for people. See https://fedmentor.dev/posts/rem-html-font-size-hack/.
  6. Get into the habit of always including a full modern css reset at the start of the styles in every project. Andy Bell or Josh Comeau both have good ones you can look up and use.
  7. Don't limit the height of elements that contain text, including the body. Use min-height instead of height. You can avoid some nasty future bugs that are hard to work out on bigger projects by doing this and establishing it as a principle now.
  8. Don't style with IDs! That's really not what they're for, makes CSS high specificity bloated and hard to manage, and can cause all sorts of referencing problems once you reach larger component based projects. Here is a detailed write up of what the ID attribute is for: https://fedmentor.dev/posts/id-attribute-masterclass/.
  9. This component is hitting the sides of my screen when it shouldn't. Make sure there is always a little space around components like this so it matches the design even on smaller screens.
  10. You can set border radius on the card itself and don't need it on the card children. Make sure you include overflow: hidden on the card so it can crop out the overflowing child corners. Also, do you definitely want border radius to be in rem so it scales with the users font size? It's fine if you do, I only mention as px is often preferred for that property so the value stays fixed.
  11. You are missing the picture element in this. You need to add this so that both images can be served correctly. The mobile image should be the src in the img element, and the desktop image in the source tag above it with a min width media query.
  12. I would expect the image to have the object-fit property set to cover and no background-clip.
  13. Make sure you understand the difference between padding and margin. It is extremely unusual for headings to have padding. They are not boxes.
  14. Speaking of the heading, I would not expect a card like this to have a h1. When we build single components like this, part of our job is to consider the context of where and how they will be used in the finished website. A h1 is for a page title (the primary/main heading for the content). But this is just a product card, likely to sit on a page with lots of product cards, which tells you it should have a lower heading level.
  15. Media queries should be defined in rem or em not px. This will make solutions more robust, ensuring designs scale and switch layout correctly, even for people who have a different text size setting.
  16. The body must never be overflow hidden. That's an extremely dangerous line and can only cause bugs.
  17. The component must not have a height (see point 7 above). Let the browser do it's job and decide what height is needed based on the content inside the card.
  18. To center a component on the screen don't use huge margins. Min-height 100svh (on the body in this case) along with some flex column properties is how to center a component in the viewport.
  19. The button hover styles need to be available on all screen sizes not just the media query. You don't know what modality someone is using when they visit your solution. I can easily see the mobile layout while on my laptop or computer and should have the hover styles available.

  I recommend you go back through all previous challenges and make sure you apply the relevant learnings there as well before moving on to anything else.

  Marked as helpful

  0