Skip to content
Submitted almost 2 years ago

qr-compoment

Wedrussowo•80
@Wedrussowo
Preview site (opens in new tab)View code (opens in new tab)
This is a solution for...QR code component
  • HTML
  • CSS
1newbie

Design comparison

SolutionDesign

Community feedback

  • Jeff Guleserian•480
    @jguleserianPosted almost 2 years ago

    Good morning, @Wedrussowo,

    Not long after you submitted this project I sat down and took a look and was preparing to send you some feedback, but then I left for church. When I got back, I noticed you resubmitted the project with some modifications. So, I’ll keep my comments mostly to this newer submission.

    Let me start by saying that the project looks amazing. You successfully made the QR Code Component look exactly like the original. Other things I really admired:

    • Looks like you figured out the alignment issue for your <div class=”frame”> container – good job! These don’t always act like we think they should. However, why didn’t you do the same thing for the <main> as a child of the div.frame, and so on?
    • You also figured out the issue that was showing up with the <h1>, though, I think their intention for the warning was for accessibility. I’ll explain below.
    • Dimensions… perfect! The only thing I could see that might need tweaking is the border radius of the actual QR code <img>.
    • I like the way you see the component’s structure as a series of stacked or concentric boxes. This type of thinking usually solves any type of design problem and groups items for styling and positioning purposes with minimal code.

    Again, great job! Admirable! Let me turn to some things that have worked for me and that I have learned are considered best practice. Keep in mind that I am no expert and that the end result of what you submitted already looks, at least on the browser, perfect!

    • Overall structure: I would probably suggest that you use more semantic HTML when you build your sites. What I mean is, using something like this…
      • <header> to house your hero image and possibly your <nav>, depending on your preference. Of course, in a component like this one, you don’t really need a <header>. Beneath it usually comes the …
      • <main> as the principal container for all content. An <h1> usually comes here or in the <header. This is divided into…
      • <section>s, each one taking on a natural division of subject. Likely each <section> would have an <h2> or other heading. Then,
      • <div>s with the elements inside them. We tend to over-utilize <div> containers and end up identifying everything by ids or classes. Using a common structure will help you as you create or revisit your work. It will also be helpful to those who come in later to collaborate or those with accessibility tools like a screen reader.
      • <footer> with the normal stuff in a footer, lol! But of course, in this component, you don’t really need it. In the end, it would look something like this: 
    <body>
  <header>
  </header>
  <main>
    <section>
      <div>
      </div>
      <article>
      </article>
    </section>
    <section>
    </section>
  </main>
  <footer>
  </footer>
</body>
    • Keep in mind that most containers are block elements. <section>, <table>, <div>, <article>, etc. This means that they take up the entire line by default – that’s why it’s easy to center stuff IN them, but not the container ITSELF. The 2 easiest ways for centering a container, in my opinion are:
      • Give it a width (so it doesn’t think it needs the whole line), and then set the right and left margins to be the same – use margin: auto or margin: 0 1fr 0 1fr, etc.
      • Go to the parent – the solution you chose for your .frame div. First, make sure the container has width (as before), then go to the parent container and use whatever means to center – I usually find myself using Flexbox and justifying/aligning the contents.
    • Accessibility: as I was mentioning earlier, the <h1> issue has to do with accessibility. Since this challenge was just component, and not a page, its whole existence depends on some other site or place that led a person to this component. The idea is that this component is not the “big idea,” it SUPPORTS the "big idea". However, it is still best practice to prompt screen readers to announce what that "big idea" topic was. Screen readers do this by locating the <h1> element first – even though it may not be listed first on the site. An easy way to handle this if you don’t have a full page with a separate topic for an <h1> (like many of the projects in Frontend Mentor) is to put an <h1> just after the <body> tag with a title such as, in this case, <h1 class=”sr-only”>Frontend Mentor Coding Camp</h1>. Then in the CSS, set the display property of display to “none” ( or you can add to that position: absolute and width: 1px and height: 1px and left: 1000px).

    Anyway, I hope you find the comments helpful. Take them or leave them. The good news it that it looks like you have some nice skills – so keep up the good work! I appreciate you letting me take a look at your work. It really helps me learn, too.

    Jeff

    Marked as helpful
    1
  • Wedrussowo•80
    @WedrussowoPosted almost 2 years ago

    Thanks, a lot for this comment! It really helps to understand more thinking process of modern frontend developers. I have couple questions related to your comment.

    if I understood it right,

    • I should use more semantic HTML and just style it with CSS? For example, instead of using <div>.frame just use <main> and style it?
    • better don't border radius the image itself, instead wrap it with the div and use the border radius on the div?

    That idea when making components and add to it an <h1> tag for sr just for training sounds very good. I'll definetly try!

    Thank You for all the time you put into helping me.

    0

Please log in to post a comment

Log in with GitHub

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