QR Code Component using HTML/CSS

Community feedback

• 

  T

  Grace•31,090

  @grace-snowPosted 7 months ago

  Hello! Well done at diving into your first challenge. there are some important learnings from this first challenge that will set you up well for future. I hope this feedback is helpful.

  1. The stylesheet link goes in the head not body.
  2. Remove the header landmark. This challenge had no header and you must never leave empty landmark elements on the page. These are very important elements for accessibility (assistive tech users use landmarks and headings to understand the contents of a page and navigate within it).
  3. This is missing a main landmark. Every page must always have a main. Change container to be a main not div. (NB: This is a single component challenge so these landmarks are not strictly needed but it's best to get into the habit of adding it straight away so you don't forget later).
  4. The image alt description isn't quite right. Think about the context of how this card component would be used on a real site. It's likely there could be several cards like this on a page so the image alt must give an accurate description of this specivific image. It should say what the image is (QR code) and where it goes (to FrontendMentor.io). There is a read post in the resources channel on discord about how to write good alt text if you want to know more.
  5. It's really valuable to make sure you Indent your code consistently so it's easier to read and spot bugs. Your code editor can even do this formatting automatically for you with prettier.
  6. The starter code came with a gitignore file. Do not remove that from your repo! It stops files like DS STORE which should not be there from being tracked by git.
  7. It is better for performance to link fonts in the html head instead of css imports.
  8. Get into the habit of including a full modern css reset at the start of the styles in every project. Andy Bell or Josh Comeau both have good ones you can look up and use.
  9. Justify content center is missing from the body in css.
  10. The components max width should be in rem not px. This is important because many people change their default text size. If you set a max width in px that is an explicit limit but would look awful and narrow for those users with larger text. If you use rem instead the layout will scale correctly for all users.
  11. I think you may be a little confused between padding vs margin. I wouldn't expect a heading to have padding as its not a box, it's a text element and has no concept of needing "internal" space.
  12. ⚠️ Font size must never be in px
  13. There is no need to use position fixed or absolute on the footer. This is causing a horrible bug where the footer overlaps the card because you're taking the footer out of its normal place in the document.
  14. I think your heading text size is too large in comparison to the design. Try to adjust that.

  Marked as helpful

  0

  ---

  

  tybaglue•40

  @tybagluePosted 7 months ago

  @grace-snow Thanks for the comments, i've tried to change the code according to ur suggestions.

  0
• 

  Boris•2,870

  @mkborisPosted 7 months ago

  Hi tybaglue great job, here are some suggestions

  • All content should be wrapped within landmarks. Wrap a main tag around the container. Also, you dont need the empty header.
  • Your css stylesheet link should be in the head not the body.
  • The body tag is the container for the content of an HTML document, so your footer should be be in the body not outside.
  • Font-size should be written in rem not px. This article explains it better Why font-size must NEVER be in pixels.

  Hope this helps.

  Marked as helpful

  0

  ---

  

  tybaglue•40

  @tybagluePosted 7 months ago

  @mkboris Thank you for the review, really appreciate it!

  0