css

Community feedback

• 

  Jake McDermott•40

  @shakey200592Posted 2 months ago

  1.Semantic HTML: The structure of the HTML is simple but could benefit from more semantic elements. Instead of wrapping everything in <div>, consider using more appropriate tags like <main> for the main content, or <section> to describe a distinct area of the page. - The heading (<h4>) should likely be a more semantically appropriate heading level such as <h1> or <h2>.

  2.Accessibility: Image alt text: The alt text "QR Code Image" could be more descriptive. Something like "QR code linking to ..." would better describe the purpose of the image.

  3.Responsiveness: Consider using percentages or relative units (vw, vh) for widths and not absolute unit sizes like px etc.

  4.Code Structure & Reusability: Using more descriptive class names (e.g., .qr-container, .qr-image, .description) could improve readability and make it clearer what each block of CSS is styling.

  Marked as helpful

  2
• 

  P

  Øystein Håberg•10,710

  @Islandstone89Posted 2 months ago

  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" section. Wrap the card in a <main>. You only need one <div> inside of <main>, that holds the card content.

  • 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". You should not include 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".

  • Headings should always be in order, so you never start with a <h4>. I would change it 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.

  CSS:

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

  • Use the style guide to find the correct font-family, and remember to specify a fallback font:font-family: 'Outfit',sans-serif;.

  • I would recommend adding 1rem of padding on the body, to ensure the card doesn't touch the edges on small screens.

  • On the body, change height to min-height: 100svh - this way, the content will not get cut off if it grows beneath the viewport.

  • Remove padding: 100px, it is not needed.

  • Remove all widths and heights in px. Setting fixed sizes in px is not recommended for web design, as we need our content to grow and shrink according to all kinds of different screens.

  • We do want to limit the width of the card, so it doesn't get too wide on larger screens. Give the card a max-width of around 20rem to solve this issue.

  • 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.

  • Increase the font size to at least 1rem, which equals 16px, for better readibility.

  • The color of the paragraph is too light - it doesn't have enough contrast against the white background. Change it to a darker color.

  • On the image, add display: block and max-width: 100% - the max-width prevents it from overflowing its container. Without this, an image would overflow if its intrinsic size is wider than the container. max-width: 100% makes the image shrink to fit inside its container.

  • I would increase the padding on the card to 16px.

  • Finally, a quick tip: whenever you're using 0 as a value, you don't need to include the unit. So, instead of 0px, just write 0.

  Marked as helpful

  0

  ---

  

  star-chris•80

  @star-chrisPosted 2 months ago

  Thank you, would appreciate more of your review in the future@Islandstone89

  1
• 

  MikDra1•6,980

  @MikDra1Posted 2 months ago

  I encourage you to use this technique to make the card responsive with ease:

  .card {
  width: 90%;
  max-width: 37.5rem;
  }
  

  On the smaller screens card will be 90% of the parent (here body), but as soon as the card will be 37.5rem (600px) it will lock with this size.

  Also to put the card in the center I advise you to use this code snippet:

  .container {
  display: grid;
  place-items: center;
  }
  

  Hope you found this comment helpful 💗💗💗

  Good job and keep going 😁😊😉

  Marked as helpful

  0
• 

  Quentin GARRAUD•30

  @quentin-garraudPosted 2 months ago

  All the elements are present but there are many differences with the original mock-up. Do not hesitate to use the indications on Figma, especially for the colors, spacing, and the size of the elements. Hold on!

  0