Desktop mode simple structure

Community feedback

• 

  T

  Grace•30,990

  @grace-snowPosted almost 2 years ago

  Hello

  This is one single component so you should only have a main element for the card. Remove footer and header and section.

  Even if it needed more landmarks this is not how you would use the section element, so I advise you avoid using it at all as you move forward on other challenges.

  Once your component is in a main landmark, change attribution to be a footer element

  Make sure you follow the style guide. The font size on this is unreadably small. The body font size should be 15px according to the style guide, which means you should have font-size: 0.9375rem (15 / 16) on the body element. That will then be the default font size for the paragraph. You will only need to set the heading size in rem judging by how large you think it looks in the design

  The card should not have a width or height. Only a max-width. The browser will decide how tall it needs to be based off the size of the content

  Last super important thing: this QR code image is extremely important content. That means it needs to be in the html with a proper alt description. It cannot be a background image

  Marked as helpful

  1

  ---

  

  Guilherme Fernandes Vidotti•520

  @VidottizzzPosted almost 2 years ago

  @grace-snow Thank you, im going to follow your advices, and improve in the next challenge!!

  0

  

  T

  Grace•30,990

  @grace-snowPosted almost 2 years ago

  @Vidottizzz you need to improve this one first. Don’t leave challenges needing more work

  0
• 

  Hassia Issah•50,650

  @HassiaiPosted almost 2 years ago

  There is no need for <div class="card-itself"> in the html and in the css, wrap the content in the main tag, remove the main tag around <h1> and remove the footer tag around p. Wrap <div class="attribution"> in the footer tag to fix the accessibility issues. click here for more on web-accessibility and semantic html

  To center the main on the page page, add min-height:100vh; display: flex; align-items: center: justify-content: center; or min-height:100vh; display: grid place-items: center to the body.

  To center the main on the page using flexbox:
  body{
  min-height: 100vh;
  display: flex;
  align-items: center;
  justify-content: center;
  }
  

  To center the main on the page using grid:
  body{
  min-height: 100vh;
  display: grid;
  place-items: center;
  }
  

  Give the main the styling you gave .card-itself but increase the width value, Increase the width value and padding value . e.g width: 320px padding:15px

  Use relative units like rem or em as unit for the padding, margin, width values and preferably rem for the font-size values, instead of using px which is an absolute unit. For more on CSS units Click here

  Hope am helpful.

  Well done for completing this challenge. HAPPY CODING

  Marked as helpful

  0

  ---

  

  Guilherme Fernandes Vidotti•520

  @VidottizzzPosted almost 2 years ago

  @Hassiai Thank you man for the feedback! Going to improve!

  0
• 

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted almost 2 years ago

  Hello there 👋. Good job on completing the challenge !

  I have some suggestions about your code that might interest you.

  HTML 📄:

  • In my humble opinion, this challenge is too small to have <header> and <footer> inside the component, it would suffice to use main for the component and the other elements can be perfectly <div>.

  <main class="card-itself">
      <div>...</div>
      ...
  </main>
  

  • Since this component involves scanning the QR code, the image is not a decoration. You must not use the background-image property to add the QR code image. Instead, use the <img> tag to add the image. Use the background-image property only for decorative images that do not add any information to the page.

  CSS 🎨:

  • Instead of using pixels in font-size, use relative units like em or rem. The font-size in absolute units like pixels does not scale with the user's browser settings. This can cause accessibility issues for users who have set their browser to use a larger font size. You can read more about this here 📘.

  • To center the component in the page, you should use Flexbox or Grid layout. You can read more about centering in CSS here 📘.

  body {
      min-height: 100vh;
      background-color: var(--Lightgray);
      display: flex;
      flex-direction: column;
      justify-content: center;
      align-items: center;
  }
  

  • You should use a CSS reset to remove the default browser styles and make your page look the same in all browsers.

    Popular CSS resets:

    • Normalize.css
    • Reset CSS
    • "My Custom CSS Reset" by JoshWComeau

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

  Happy coding!

  1

  ---

  

  Guilherme Fernandes Vidotti•520

  @VidottizzzPosted almost 2 years ago

  @MelvinAguilar Thank u for the rich feedback, im going to improve in the next!

  0