Frontend-Mentor - qr-code-component-solution

Community feedback

• 

  Jon Hill-Upperton•150

  @JonhilluppertonPosted over 2 years ago

  Hello Can_Ceylandag,

  Great on submitting your solution.

  On mobile version it is all pushed together.

  What you can do is in the body tag of the css.

  Make sure you have Min-Height:100vh, flex-direction as column, justify-content and align-items as center. Don’t put a width in the body tag

  Then on the card itself put a max-width of 320px. Removing the margin properties you use to move it down and long the page.

  Also you don’t need a media query for this. In the HTML on the first line where it mentioned about Improve your skills make this a H1.

  Thanks

  Jon

  Marked as helpful

  1
• 

  Ahmed Bayoumi•6,740

  @Bayoumi-devPosted over 2 years ago

  Hey!

  My suggestions:

  • Document should have one main landmark, Contain the component with <main>.

  <main>
     <div class="container">
        //...
     </div>
  </main>
  

  • All page content should be contained by landmarks, Contain the attribution with <footer>.

  - `Page should contain a level-one heading`, Change `<p class="upperText">` to `<h1 class="upperText">` You should always have one `h1` per page of the document... in this challenge, you will use `h1` just to avoid the `accessibility issue` that appears in the challenge report... but don't use `h1` on small components `<h1>` should represent the main heading for the whole page, and for the best practice use only one `<h1>` per page.
  
  <footer>
     <div class="attribution">
        //...
     </div>
  </footer>
  

  • I suggest you center the component on the page with Flexbox, by giving the parent element <main> the following properties:

  body {
    /* max-width: 1440px;      <---Remove */
    //...
  }
  main {     /* <--- Add */
     display: flex;
     justify-content: center;
     align-items: center;
     min-height: 100vh;
   }  
  
  .container {
    /* margin-top: 5%;      <---Remove */
    /* margin-left: 36%;      <---Remove */
    padding: 1%;
    /* width: 25%;      <---Remove */
    max-width: 320px;     /* <--- Add */
    border-radius: 4%;
    background-color: hsl(0, 0%, 100%);
  }
  

  Hope this is helpful to you... Keep coding👍

  Marked as helpful

  0
• 

  Jon Hill-Upperton•150

  @JonhilluppertonPosted over 2 years ago

  Also in the body put a display: flex as well. The attribute div to be put in a footer.

  1