QR code component - kindly review the code.

Community feedback

• 

  Ecem Gokdogan•9,380

  @ecemgoPosted over 1 year ago

  Some recommendations regarding your code that could be of interest to you.

  HTML

  In order to fix the accessibility issues:

  • You need to replace <div class="main"> with the <main class="main"> tag. You'd better use Semantic HTML, and you can also reach more information about it from Using Semantic HTML Tags Correctly.
  • Each main content needs to include at least h1 element so you should use one <h1> element in the <main> tag. You can replace your <h2>Improve your front-end skills by building projects</h2> element with the <h1>Improve your front-end skills by building projects</h1> element.

  After committing the changes on GitHub and you need to deploy it as a live site. Finally, you should click generate a new report on this solution page to clear the warnings.

  CSS

  • If you want to make the card centered both horizontally and vertically, you'd better add flexbox and min-height: 100vh to the body

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

  • If you use flexbox in the body, you don't need to use margin in the .main to center the card
  • If you use max-width, the card will be responsive and you can reduce the width a bit to look better

   .main {
      /* width: 375px; */
      max-width: 300px;
      /* position: relative; */
      /* margin: 200px auto; */
  }
  

  Hope I am helpful. :)

  Marked as helpful

  1

  ---

  

  Alias Kevin Kunju•10

  @aliaskevinPosted over 1 year ago

  @ecemgo , that was really informative. thanks for your taking time to explain.

  1

  

  Ecem Gokdogan•9,380

  @ecemgoPosted over 1 year ago

  @aliaskevin happy to help :)

  1
• 

  Abdul Khaliq 🚀•72,560

  @0xabdulkhaliqPosted 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.

  ---

  CSS 🎨:

  • Looks like the component has not been centered properly. So let me explain, How you can easily center the component without using margin or padding.

  ---

  • We don't need to use margin and padding to center the component both horizontally & vertically. Because using margin or padding will not dynamical centers our component at all states

  ---

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

  ---

  • For this demonstration we use css Grid to center the component.

  body {
      min-height: 100vh;
      display: grid;
      place-items: center;
  }
  

  • Now remove these styles, after removing you can able to see the changes

  .main {
    margin: 200px auto;
  }
  

  ---

  ---

  • Now your component has been properly centered

  ---

  .

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

  Happy coding!

  Marked as helpful

  1

  ---

  

  Alias Kevin Kunju•10

  @aliaskevinPosted over 1 year ago

  I read and noted all the suggestions, thank you @0xAbdulKhalid

  0
• 

  Pavel B.•270

  @JagholinPosted over 1 year ago

  Great job!

  Some suggestions for the future:

  • inlining CSS styles into HTML can be sometimes useful(from performance standpoint). However I suggest to forget that it's even possible and add links to .css files instead. In the future when you do larger projects, separating stuff into different files will help you with organization, and bundlers like Vite will take care of compiling your code into optimised, minified representations ready to deploy to the Web.
  • position: relative doesnt do anything here.
  • The task was to display the panel in the center of viewport, which is done only 50%. There are several ways of vertically and horizontally centering stuff, the one that I like is to add this

  body {
    height: 100vh; /*if the browser doesnt understand the next line*/
    height: 100dvh; 
    display: grid;
    place-content: center;
  }
  

  accordingly, you dont need such large margins on top and bottom of your .main

  (in fact, these margins break layout for a range of heights).

  • instead of using width, I suggest using max-width. This simple change will make your layout more fluid and responsive to wider range of viewport widths.

  Marked as helpful

  1

  ---

  

  Alias Kevin Kunju•10

  @aliaskevinPosted over 1 year ago

  Thank you for your informative advice @Jagholin

  0