Responsive-Product card

Community feedback

• 

  Enmanuel Otero Montano•2,155

  @Enmanuel-Otero-MontanoPosted over 2 years ago

  Hello Stalin!

  In general your HTML is fine, but I would like to give you a couple of suggestions for improvement.

  Try to use semantically meaningful tags (main, section, article, etc) instead of div, I'm not saying don't use div, but only when necessary. What are the benefits of writing semantic HTML? Well, several, more structured and easy-to-read HTML, gives the browser more information about your web page, which gives you a better chance of obtaining better positioning, improves accessibility, among other benefits. What would you change in your code to make it more semantic? Well, the div with the product-container class would change it for a main tag with the same class that the div has, the other thing that would change would be the div with the details class for a section or article tag with the same class.

  Finally, when you have to import or call more than one font, do it in a single call, I'm telling you about it because you have two requests and that decreases performance.

  Any questions you can ask.

  Greetings!

  Marked as helpful

  0

  ---

  

  Stalin•410

  @StalinAMPosted over 2 years ago

  @Enmanuel-Otero-Montano thanks for your comment, I hadn't realized about the fonts and the html structure, I will take it into account for future projects. Thank you very much for your comment.

  0
• 

  Elaine•11,400

  @elaineleungPosted over 2 years ago

  Hi Stalin, this looks good on the whole, so well done, first of all!

  About the HTML, I think things look OK here! I might do one or two things differently, but no major issues. For the perfume image, I probably would give it a more descriptive alt tag than just "perfume" (maybe "bottle of perfume with some leaves in the background"?). Also, your classes are clearly named in general (as in, not box-1); the only thing I'd do differently is, for the details, I probably would call it details-container only because its sibling is called a container, and so it'd be easier for me to see that they're related.

  About the CSS, you really did many things well, the most important of which is, you did not set fixed widths on the selectors and used padding and margins instead for sizing and positioning. The only thing I couldn't figure out at first was why the two containers weren't equal; it didn't make sense when you used 1fr 1fr for the grid columns and you already didn't have any fixed widths. I finally realized what was going on: the problem here is with the grid column values you used in product-price. Instead of grid, I'd use flexbox, and I'd wrap the big price in span tags as well so that I can use align-items: center to make both prices vertically centered, and then just add a little margin to give them some space. After that, the two image and text containers should be equal in desktop view!

  Anyway, hope this feedback can help you out a bit, and really great job here once again! 🙂

  Marked as helpful

  0

  ---

  

  Stalin•410

  @StalinAMPosted over 2 years ago

  @elaineleung thank you for your comments, I will take them into account in future projects.

  1