Qr-code component-main-challenge

Community feedback

• 

  P

  Jeronimo Palacios•110

  @giropa832Posted 3 days ago

  Hi,

  This is a great first approach. I am reviewing your solution and I'd like to point some details:

  • Look at the design system (fonts, colours)
  • Try to make the design as pixel perfect as possible. Sizes don't correspond to each other.

  Maybe try to do it with Vanilla CSS and HTML first?

  I wish you the best of luck

  Marked as helpful

  1
• 

  P

  Øystein Håberg•10,570

  @Islandstone89Posted 3 days ago

  Hey, good job!

  Here is my feedback - I hope it is clear and helpful :)

  HTML:

  • Every webpage needs a <main> that wraps all of the content, except for <header> and footer>. This is vital for accessibility, as it helps screen readers identify a page's "main" content. Change .wrapper to a <main>.

  • The image has meaning, so it must have proper alt text. Write something short and descriptive, without including words like "image" or "photo". Screen readers start announcing images with "image", so an alt text of "image of qr code" would be read like this: "image, image of qr code". The alt text must also say where it leads(the frontendmentor website). A good alt text would be "QR code leading to the Frontend Mentor website."

  • I would change the heading to a <h2> - a page should only have one <h1>, reserved for the main heading. As this is a card heading, it would likely not be the main heading on a page with several components.

  • .attribution should be a <footer>, and you should use <p> for the text inside.

  CSS:

  • Including a CSS Reset at the top is good practice.

  • The font is not showing because you have a typo in your CSS:

  --ff: 'Outfit'. 'inter', sans-serif;
  

  must be changed to:

  --ff: 'Outfit', 'inter', sans-serif;
  

  • I recommend adding a bit of padding, for example 16px, on the body, to ensure the card doesn't touch the edges on small screens.

  • Move the styles on .wrapper to the body. Change height to min-height: 100svh - this way, the content will not get cut off if it grows beneath the viewport. Add flex-direction: column, justify-content: center and gap: 2rem.

  • Remove margin-inline: auto on the card ,it is not needed, since it is centered using Flexbox.

  • max-width on the card should be in rem. Around 20rem will work fine.

  • font-size must never be in px. This is a big accessibility issue, as it prevents the font size from scaling with the user's default setting in the browser. Use rem instead.

  • Since all of the text should be centered, you only need to set text-align: center on the body, and remove it elsewhere. The children will inherit the value.

  • You can remove font-weight: 400 on the body, as that is the default value.

  • The paragraph text has poor contrast - if you hover over it in DevTools, it shows a contrast ratio of 3.62. It must be at least 4.5 to fulfill the minimum requirements from the Web Content Accessibility Guidelines.

  • On the image, add height: auto, and change display: inline-block to display: block. Add a border-radius of for example 16px to give it rounded corners. Finally, add margin-bottom: 1rem to create breathing room between the image and the text.

  Marked as helpful

  0