Responsive Mobile-first Product Preview Card Component

Community feedback

• 

  T

  Grace•31,250

  @grace-snowPosted 17 days ago

  I hope this is helpful:

  • Try to avoid ever placing text in meaningless elements like divs or spans. Instead use meaningful elements every time even if that’s just a paragraph. Learning should be a paragraph in this. So should the main paragraph of text and price.
  • Remove the BR elements. That isn’t how you create line breaks. Let line brakes happen naturally based off the available space. The BR element will be announced by some screen readers as "Break" Which isn’t what you want.
  • The old price should be in a strike through element (s). You will also need to add some visually hidden text in a span to let screenreader users know that that is the old or pre-sale or original price. Without that, they will hear both prices and that may be confusing.
  • The alt description on the main image should describe what this product looks like and not just repeat the product name. There are some great articles in the discord Server about how and when to write alt text on images. Take a look at the resources channel.
  • On a similar point, the cart icon is a decorative image so the out text should be left intentionally blank.
  • It’s better for performance to link fonts in the HTML head instead of using CSS imports.
  • get into the habit now of always including a full modern CSS reset at the start of the styles in every project you ever do. Andy Bell has a very good one with a blog explaining why he’s included what he has. Read that and then use that CSS reset. Alternatively Josh Comeau Also has a good modern CSS reset which you may prefer.
  • Font size should never be in pixels. Use rem instead every time. More info: https://fedmentor.dev/posts/font-size-px/
  • The card should have a max width in Rem. This then becomes a larger max with in the media query for larger screen sizes.
  • I think you’re getting confused between padding and margin and When to use each. I've Written a blog about this which you may find helpful: https://fedmentor.dev/posts/padding-margin/
  • what you have now is definitely not wrong, but it’s just a bit unusual. Line height is usually unitless not in rem. Eg 1.5.
  • if using flex box use the gap property to create space between the child elements instead of placing directional margin on those child elements.
  • Don’t remove the border altogether from buttons. Windows high contrast mode relies on buttons still having a border to be able to identify them. Instead always keep the border but you can make it the same colour as the background of the button or you can make the Border colour transparent.

  ask for your question about flex box. Personally, I would use grid for this challenge and I would recommend that that’s what you do, which is exactly what you’ve done. It’s short of CSS. It would be to achieve the same thing with Flex box.

  But just so that you know how you would do it with flex box, You would need to use all the flex propertie. By that I mean, grow shrink and basis. Eg flex: 0 0 50% On each child element of the card. That’s longer than just setting grid and the grid template columns to one FR each

  Marked as helpful

  1
• 

  P

  Suhas Patnaik•120

  @SuhasPatnaikPosted 16 days ago

  Thanks a ton for your comment, Grace.

  I really appreciate the time you have taken to study my code and give in-depth feedback to create a habit of best practices and better web accessibility.

  I have integrated most of the changes into this project and will ensure that other points are integrated into upcoming projects.

  0