QR Code solution

Community feedback

• 

  T

  Grace•30,990

  @grace-snowPosted 8 months ago

  Hi @Jonee2, here is some feedback as promised. Take this and apply to all of your solutions

  1. As others have said, all content must be contained within landmarks. This page should have a main for the component (card) and a footer for the attribution.
  2. Inside main have a div to be your component. It does not need to be a flex column as all of it's contents would stack by default anyway.
  3. Use a heading and paragraph.
  4. Note the <b> tag is deprecated and meaningless. Don't use it.
  5. Img elements must always have an alt attribute. This is extremely important. Read the excellent post in the discord resources channel about how and when to write alt text. In this challenge the image is very important content so must include what the image is (QR code) and where it goes to (to the Frontend Mentor website)
  6. NEVER write font site in pixels. Use rem. Read the post and make sure you understand why this is so important.
  7. Don't use large margins to try and build a layout. Margins and paddings should always be small. To center the component on the page use flex properties just as you have done, but also include min-height 100vh (or svh) so the body extends to at least the screen height. By default the body is only as tall as its content.
  8. Always use a modern CSS reset at the start of the styles in every project. Get into the habit now! Andy Bell and Josh Comeau both have good examples online you could use. This will set sensible defaults, set images to display block and iron out discrepancies between different browser's default styles.
  9. You have made all of this tiny. The style guide gives the base font size and that is what the paragraphs in the design should look like. Don't make them smaller!
  10. Make sure you understand the difference between padding and margin. The card needs padding (internal spacing to create space inside it). The card's child elements only need vertical margins to (external spacing to create space between them)
  11. The image must not have an explicit width. It should be display block and max-width 100% (both a standard part of any modern CSS reset).
  12. The card should not have a width and it's max width should be in rem. I advise you steer clear of key words like min-content and max-content and fit-content. They are advanced concepts and you will rarely, if ever, need to use them.
  13. The card does not need a height. It's the browsers job to decide how much height the component needs, so let it do its job.
  14. Border radius should be in a fixed amount not in percentage. Percentage will only work on square / perfectly circular radius it will cause bizarre corners on rectangles so is best avoided in cases where you want slightly rounded corners.

  1
• 

  P

  James Rodriguez Flores•340

  @james-rodPosted 9 months ago

  Hi Jhonne,

  I want to say great job on this challenge! If you want to make sure that you that your container is center perfectly, here's what you should do inside your <main> or <div>

  <main>
     // Rest of your code
  </main>
  

  main{
      display: flex;
      justify-content: center;
      align-items: center;
      min-height: 100vh;
  }
  

  This will allows anything inside you main tag to be center when you use flexbox or grid!

  Hope this helped!

  1
• 

  Daniel 🛸•44,250

  @danielmrz-devPosted 9 months ago

  Hello @Jonee2!

  Your solution looks great!

  I have a couple of suggestions (about semantic HTML) for improvement:

  📌 First: Use <main> to wrap the main content instead of <div>.

  Tags like <div> and <span> are typical examples of non-semantic HTML elements. They serve only as content holders but give no indication as to what type of content they contain or what role that content plays on the page.

  📌 Second: Use <h1> for the main title instead of <span>.

  Unlike what most people think, it's not just about the size and weight of the text.

  • The <h1> to <h6> tags are used to define HTML headings.
  • <h1> defines the most important heading.
  • <h6> defines the least important heading.
  • Only use one <h1> per page - this should represent the main heading/title for the whole page. And don't skip heading levels - start with <h1>, then use <h2>, and so on.

  All these tag changes may have little or any visual impact but they make your HTML code more semantic and improve SEO optimization as well as the accessibility of your project.

  I hope it helps!

  Other than that, great job!

  1

  ---

  

  T

  Grace•30,990

  @grace-snowPosted 8 months ago

  @danielmrz-dev this is a card component that would sit within a web page, it is not a full page. As it would never serve as a page title, the h1 is not an appropriate heading level for this component. H2 would be more appropriate.

  1
• 

  Faruq Abdul Hakim•580

  @faruqAbdulHakimPosted 9 months ago

  Hi Jhonne 👋,

  I have some suggestions for you to center the component. First of all, make sure your document is filled 100% of screen height by applying:

  body {
      min-height: 100vh;
  }
  

  and then, if you want to add attribution (optional) make sure it doesn't affect the component by applying:

  .attribution {
      position: absolute;
      bottom: 0;
  }
  
  Hope this can help. 😃
  

  1

  ---

  

  T

  Grace•30,990

  @grace-snowPosted 8 months ago

  @faruqAbdulHakim don't set the attribution to be position absolute. That takes it out of the documument flow and can cause overflow bugs, especially on smaller screens. It's much better to leave the footer in the normal document flow and let it sit directly below the component.

  If you really want a component centered and the footer right at the bottom of the page you would

  • make the body a flex column with min-height 100vh (or svh)
  • make the main flex-grow 1 so it stretches to take up as much height as possible and push the footer to the bottom.
  • make the main into another flex column and use align-items and justify-content to center the card.

  0