Product Preview Card Component using Flexbox

Community feedback

• 

  T

  Grace•31,270

  @grace-snowPosted about 1 year ago

  It's worth having a read of this post I wrote about planning html which looks specifically at this challenge. I recommend you update your html and this will explain why/how.

  It's also worth reading about why font size (and letter spacing and line height) must never ever be in px which is explained on another post on the site.

  You need to remove the widths and heights in this solution. It is making it inaccessible. The component should only have one max width.

  The img can be width and height 100% along with display block and object-fit (the img should be one img inside the picture element not two img elements)

  The button should not have an explicit width and height either. Width 100% and padding only

  Really important - the media query should be defined in rem or em and should be set at a point where the layout needs to change. That will be much larger than 375px. There is no room for the desktop layout at 376px.

  On your next project (or on this one if you really want to do it right) make sure you are working mobile first. Mobile styles should always be the default.

  Definitely don't style on IDs. There's a post all about that on the site too.

  Really this is not the right challenge to be doing as a first one. Most of these issues get ironed out in the QR code challenge. Doing them in a good order will help you learn progressively and prevent the feedback amount from being overwhelming (again there is a post suggesting an order for these challenges)

  Marked as helpful

  1

  ---

  

  MeaganA•40

  @MeaganAPosted about 1 year ago

  @grace-snow Hello Grace! Thank you for your feedback. I agree that I should start with the QR code challenge first. On my next challenge, I will take your and @claudionetto advice to do away with the ID selectors. I started building this challenge desktop-first instead of mobile-first, so I figured out why I mainly used width, height, and pixels for sizing certain elements on this challenge. So I am definitely to work on that. Thank you again for this feedback. I know I still have a lot to learn, but I am up for a challenge. :)

  0
• 

  Rodrigo•480

  @RodrigoHLCPosted about 1 year ago

  Not much to add, looks very good to me! The only minor detail that's missing would be setting the cursor to a pointer when hovering over the "Add to cart" button.

  Marked as helpful

  0

  ---

  

  MeaganA•40

  @MeaganAPosted about 1 year ago

  @RodrigoHLC Hello Rodrigo. Thank you for your feedback. I didn't know there was a cursor property. So after I read your comment, I went to Mozilla Web Docs and found the cursor property. Now, I have it saved as a bookmark for future use. Thanks for this this! :)

  0
• 

  scorpio•170

  @ScorpiojkPosted about 1 year ago

  It's a better solution to not use padding. You can center elements in a more responsive way eg. using flex. This wil made easier to do the borders because you'll have a container for the product. Good luck and success in your next works!

  Marked as helpful

  0

  ---

  

  MeaganA•40

  @MeaganAPosted about 1 year ago

  @Scorpiojk Hi Scorpio! Thank you for your feedback. I was juggling between using margin and padding for centering the card component in this challenge. However, I will take advantage of Flexbox more for the next challenge. :)

  1
• 

  Claudio Netto Junior•180

  @claudionettoPosted about 1 year ago

  I'm also new to programming, but one tip I can give you is to avoid using id for selectors, mainly use the class, there are some reasons for this but I'm not the best to explain them here quickly so I recommend doing some research, I'll also leave a link to a discussion about it and I advise you to read it:

  https://dev.to/clairecodes/reasons-not-to-use-ids-in-css-4ni4

  Now according to the solution itself, it looks pretty good, the only thing missing apparently is to reduce the line height of the h1.

  Marked as helpful

  0

  ---

  

  MeaganA•40

  @MeaganAPosted about 1 year ago

  @claudionetto Hello Claudio! Thank you so much for reviewing my code. I agree that I was a little concerned about using the ID selectors for some of my elements. However, I am going to start using the class selectors instead from this day forward. As for the line height in the h1, I will make improvements on this as well. Thank you so much for your feedback and for the link to the article above. Trust me, it was very helpful! :)

  0