QR code challenge CSS solution

Community feedback

• 

  Aayush Jaiswal•860

  @aay7ushPosted over 1 year ago

  Congratulations 👏 Laryssa Carvalho on completing your first challenge! You have shown great efforts in developing your code.

  I have some suggestions on your HTML code. Firstly, I would recommend using the <main> element to wrap all the main content of your webpage. Additionally, try using semantic HTML elements like <article> and <section> instead of generic element like <div> to improve the structure and meaning of your code.

  I noticed that you have an image of a scanned QR code. It is important to include an alt attribute on the <img> element that describes the purpose of the image for accessibility reasons.

  Moving on to your CSS, I suggest using BEM naming conventions for your classes. This naming convention helps to keep your CSS organized and readable, making it easier to maintain and update your code.

  When applying font families, it is a good practice to use fallback fonts after the main font to ensure that your text will still be readable even if the main font is not available. To center all the content of a <main> element, you can use the display: grid and place-content: center properties on body element. You can also directly apply the text-align: center property to the body element instead of individually applying it to every element.

  If you are trying to center an element horizontally, you can use the margin-inline shorthand property instead of margin: 0 auto. Additionally, I recommend using relative units like rem or em instead of absolute units like pixels, as they are more scalable and responsive to the user's browser settings.

  Keep up the good work and happy coding!

  Marked as helpful

  1
• 

  Tobenna•610

  @tobezhanabiPosted over 1 year ago

  Hello there 👋. Congratulations on successfully completing the challenge! 🎉

  I have other recommendations regarding your code that I believe will be of great interest to you.

  HTML 🏷️:

  This solution generates accessibility error reports due to non-semantic markup, which lack landmark for a webpage

  So fix it by replacing the element <div class="container"> the with semantic element <main> in your index.html file to improve accessibility and organization of your page. This is known as landmark

  What is landmark ?, They used to define major sections of your page instead of relying on generic elements like <div> or <span> They convey the structure of your page. For example, the <main> element should include all content directly related to the page's main idea, so there should only be one per page

  HEADINGS ⚠️:

  This solution also generated accessibility error report due to lack of level-one heading <h1> Every site must want at least one h1 element identifying and describing the main content of the page. An h1 heading provides an important navigation point for users of assistive technologies, allowing them to easily find the main content of the page. So we want to add a level-one heading to improve accessibility by reading aloud the heading by screen readers, you can achieve this by adding a sr-only class to hide it from visual users (it will be useful for visually impaired users)

  *I hope you find it helpful ! 😄 Above all, the solution you submitted is great

  Happy coding!

  Marked as helpful

  1

  ---

  

  Laryssa Carvalho•30

  @laryssacarvalhoPosted over 1 year ago

  @tobezhanabi thanks for the feedback 😄 just one question about the heading, I could just replace my h2 element for a h1 element, instead of creating a class to hide it, right? Or do I need to have h1 and h2 elements?

  0

  

  Tobenna•610

  @tobezhanabiPosted over 1 year ago

  @laryssacarvalho Yeah, you just need h1. One of the reason h1 is necessary is because of Google crawler. It helps your post become accessible. You don't need to make a h1, and h2

  Marked as helpful

  0
• 

  P

  visualdennis•8,375

  @visualdennissPosted over 1 year ago

  Hey Laryssa,

  Your solution looks great! Your way of centering is perfectly valid, you have seem to used margin: 0 auto to center the whole container that contains the card. Since it is already using as little space as possible horizontally now (due to margin: 0 auto), your container has actually a width of 324px and 100% of the body, which is close to full viewport height. In this case, align-items: center; of .container actually has no actually no effect on centering the card, only the justify-content: center; has an impact (aligning it vertically - since it is flex-direction: column and main-axis is now a vertical line). So you might actually delete align-items:center inside the .container and it would still work same : )

  Hope you find this feedback helpful!

  Marked as helpful

  1
• 

  G•80

  @gmena-albPosted over 1 year ago

  Hi Larisa! How you did is cool. I like to use display grid and place content center.

  1