Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found

Submitted

QR code component - kindly review the code.

@aliaskevin

Desktop design screenshot for the QR code component coding challenge

This is a solution for...

  • HTML
  • CSS
1newbie
View challenge

Design comparison


SolutionDesign

Solution retrospective


Hi there, Your feedbacks and suggestions are much appreciated.

Community feedback

Ecem Gokdogan 9,380

@ecemgo

Posted

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

@aliaskevin

Posted

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

1
Ecem Gokdogan 9,380

@ecemgo

Posted

@aliaskevin happy to help :)

1

@0xabdulkhaliq

Posted

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

@aliaskevin

Posted

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

0
Pavel B. 270

@Jagholin

Posted

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

@aliaskevin

Posted

Thank you for your informative advice @Jagholin

0

Please log in to post a comment

Log in with GitHub
Discord logo

Join our Discord community

Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!

Join our Discord