Responsive Product Card

Community feedback

• 

  T

  Gwenaël Magnenat•1,290

  @gmagnenatPosted about 1 month ago

  Hi, congrats on taking this challenge!

  I notice some important points you'll want to fix. It will help you and make your life much easier when styling and later when using JavaScript.

  Does the solution include semantic HTML?

  • It's better to load your stylesheet after the fonts. The risk here is loading some styles without the right fonts.
  • You need to read about landmarks and when to use which (<main>, <header>, <footer>). <main> represents the dominant content of the page. <header> and <footer> need to be outside of <main>. If you add them in the main landmark, they act like a div and are completely useless. Each of these landmarks has an implicit role when they are used right. A header landmark has a "banner" role and contains content that is repeated on different pages such as navigation. There is no need for a header in this solution. As the footer needs to be outside of <main>, it can usually contain the attributions and links in these challenges. Otherwise, you don't really need a footer on this component.
  • About the use of section: it represents a section of a document and needs a heading. As you only have one here and don't need to differentiate several sections, a div is perfectly fine.
  • The main title of the component is the product name, so this should be your heading. For the perfume overline, you can use a <p> tag. Also, I recommend you don't use capital letters in your HTML but style this with CSS to put the text in caps. As it is a component that will probably be displayed on a page with other similar components, it's better to use an h2, as the h1 will be in another place on that page. You can ignore the HTML validation on the form or use a hidden h1 if you want.
  • For the price part, if you are a screen reader user, this part is confusing as it announces two prices without a difference between them. You can use the <del> element to wrap the previous price and also add a screen reader-only text on that element that says, for example, "old price was:". It adds more context to what is announced to non-sighted users.
  • The image on the button is purely decorative and doesn't add more information to this button for non-sighted users. You need to leave the alt text empty so the screen reader skips that icon and doesn't announce it.

  Is it accessible, and what improvements could be made?

  • You need to consider users who are changing their browser's default font size. The default font size of the browser is 16px, but some users change this value to display websites with larger text. You can read this great article that explains why you should not use pixels for font sizes. It gives examples and techniques on how to test your code.
  • On the same topic, you always want to use relative units such as rem for your media queries. As the user can change the default browser font size, you want to respect these settings and switch your layout at these updated values. Pixels don't let you do that. This counts for your media query but also for the switch of your picture element.
  • You need to add a modern CSS reset at the top of all your stylesheets in every project. Look up Andy Bell or Josh Comeau; they both have great modern CSS resets (and also super great blogs). Make sure to understand what each line of these resets does. It will help you write strong styles from the start instead of fighting with some browser inconsistencies.
  • You can remove the height of 100% on body and html. It's already 100% height and width by default.
  • It is recommended to use a unitless value for line-height to avoid unexpected behavior unitless line height: MDN.
  • You should use relative units on your min-width such as rem. If you test your solution by increasing the font size of your browser to 40px, you will see that your solution breaks. It needs to respect the user's settings.
  • Instead of using !important values, you can use flex-box, max-width on your image, and other techniques to have a more flexible and solid solution.

  Does the layout look good on a range of screen sizes?

  • It looks good and respects the design. Some areas can be improved for better accessibility.

  Is the code well-structured, readable, and reusable?

  • I recommend you structure your CSS from top to bottom with different sections. Currently, it's a bit unordered, going from button to main, then picture, and so on. Try to follow the visual order of the design and structure your stylesheet in that order. It will help others read and understand your code quickly.
  • Try to focus on the lowest specificity of your selectors and increase only if really needed. Add classes to every element you want to style. It will help you build a more maintainable codebase. If tomorrow you change the HTML, the stylesheet stays the same, but if you style directly on the HTML elements, you need to rewrite the selectors at each change.

  Does the solution differ considerably from the design?

  • It looks good, good job! Just the important things about HTML you want to check. You are overcomplicating stuff currently. It can be much cleaner, more solid, and accessible.

  I hope you find some useful informations and tips in this review and I hope it helps you better understand that good HTML is a very important fundamental skills that helps build solid and accessible solutions.

  Let me know if you need more clarifications on one of the topic.

  Happy coding !

  1
• 

  AlexisParder•70

  @AlexisParderPosted about 1 month ago

  good job

  1