Skip to content
OverviewSolutions
2
Comments
2
Jason Ritter
@Higokian

All comments

  • Jason Ritter•40
    @HigokianPosted over 1 year ago

    Great work! I'm only noticing two things you could take a look at.

    Size

    Like previously mentioned, reducing the size of the card would be good to better match the intended use case.

    Semantic HTML

    Instead of using a bunch of divs, you could change the qr-code-box div a <main></main> element and the attribution div could be a <footer></footer>.

    Over-all, this is a great solution. Your code is clean and very well structured and you managed to get spacing within the box to appear spot on!

    Here is a great resource for learning semantic HTML if you're unfamiliar with it: W3Schools Semantic Elements

    0
  • Jason Ritter•40
    @HigokianPosted over 1 year ago

    Great job on your first project! I just have a few suggestions.

    #bold id in <p> element

    I suggest making this a class instead of an id, this would be great for reusability. Another option would be to use the <strong></strong> tag to bold.

    main class

    Rather than giving this div a class of main, you could change the div to a <main></main> element. This would be good for structure and accessibility for things such as screen readers.

    attribution class

    You could change this div into a <footer></footer>. This has similar benefits to using the main element.

    External CSS

    To aid with code readability and file structure, I recommend using an external css file rather than using internal css. Not only does this benefit file structure, it's also good for reusability. This would allow you to link the css file to other html files rather than having to copy a lot of css into each html file.

    Centering QR Code Component

    I love your use of flexbox! It seems to me you have a pretty good grasp of it. One thing that may help with getting it to center within the viewport without having to scroll down is to give the body a height: 100vh. This will make the body take up 100% of the viewport no matter the device. If you then get rid of the 670px margin in your main class and the -700px margin in your attribution class, this should make all of your content center without scrolling!

    I really hope that this is helps! Keep up the great work!

    Happy coding!!

    Marked as helpful
    1