Finished QR code component

Community feedback

• 

  T

  Grace•31,270

  @grace-snowPosted 5 months ago

  Here is some foundational feedback to help you. Note you will need to refactor all projects with some of these things.

  1. All content should be contained within landmarks. This needs a main wrapping the component, and a footer for the attribution.
  2. You don't need to use the picture element in this. The picture element is used when you want to serve different images at different screen sizes. This only has one image. When you do need to use the picture element in future, you would make the mobile image the default inside the img element and you would use a min-width media query in the source tag (media query defined in rem or em not px) to serve the larger screen image.
  3. The Imogen this is extremely important content. That means it needs a proper description. In this case the alt should say what the image is (QR code) and where it goes to (two frontendMentor.io).
  4. There should be no hgroup in this.
  5. You don't actually need an extra div to wrap the image or the text. It's no problem having it in there but as a general rule keep the HTML as simple as possible.
  6. Remove the br from the heading. That is not how you create line brakes. Some screen readers will announce this as "break" in the middle of the heading, which is not what you want. Instead let the lines break naturally. In some cases in certain challenges if you need lines to break earlier you would give the element a max width rem or ch units, and margin-inline auto if that limited width element also needs horizontal centering.
  7. Never set font-size in px. Use rem.
  8. Get into the habit of always including a full modern CSS reset at the start of your styles. Andy Bell or Josh Comeau both have good ones you can look up and use.
  9. It's better for performance to link fonts in the HTML head instead of CSS imports.
  10. Recommend use the flex column method to centre components like this in the viewport and not the grid place-content center method. This will prevent overflow people viewing a site on smaller screens who have a larger size. It's a more robust method.
  11. There should be no widths or heights in this challenge. All the card needs is a single max width in rem.
  12. The image must not have a width pixels. All it needs is what is included in a CSS reset anyway (Display block and Max width 100%).
  13. There is no benefit to using grid on this card. All it needs is a text align center. The only benefit using grid or flex on this card would be if you wanted to use the gap property instead of margin to create vertical spaces. But really that's unnecessary.
  14. The image doesn't need small padding and margins. Let the image be Full size. Can optionally have a margin bottom. But as you've already given margin top to the H2 even that is unnecessary.
  15. Your card is actually off centre at the moment. This is because the footer (attribution) is wider than the card and the card sits to the left inside it's grid area. If you use the flex column approach to center your component as I suggest above this would be solved.

  0
• 

  Natércio•30

  @Natercio8Posted 6 months ago

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

  0

  ---

  

  Randomgituser69•230

  @Randomgituser69Posted 3 months ago

  @Natercio8 yes

  0
• 

  P

  gonzzdev•70

  @gonzzdevPosted 6 months ago

  Hey there!

  Here are some steps on how to import your image.

  • Go to your GitHub repo (https://github.com/Randomgituser69/Qr-code-component-)
  • Click on add file and drag the file to your repository.
  • Edit your index.html file to add image using:

  <img src="image-name.png" />
  

  0