Skip to content
Submitted almost 3 years ago

QR Code component

Myndi•20
@Myntsu
A solution to the QR code component challenge
Preview (opens in new tab)Code (opens in new tab)
Code

Please log in to post a comment

Log in

Community feedback

  • Yari Morcus•500
    @YariMorcus

    Congratulations on completing this challenge Myndi!

    I took a look at your solution, and I need to say that it looks pretty good! I can see that you took the time to create your component as close as looking to the designs that where given to you. This is not something I see from all people, so you should be proud on yourself!

    Of course I do have some feedback for you, which I have been dividing into sections for more structure.

    Section: HTML - feedback

    1. Try to be as specific as possible in your <html lang="en"> attribute. So in this case, I would use: <html lang="en-us">. This is important for not only search engines, but also assistive technologies such as screen readers and browsers. Browsers use this tag to make sure it is using the correct dictionary in order to apply the right grammar and spelling (yes, there is a difference, you can consult the article of The Cold Wire for more information). Screen readers, for example, use this tag to apply the correct pronunciation of words.

    2. Try to use the semantic HTML 5 elements instead of the common block-level element called div. A div (just like a span) does not convey any information to search engines and assistive technologies about what the content really is (unless you use ARIA, but that is something you should not be concerned with at the moment). What I did was the following: I added <main> as a direct child of <body> to indicate that this is the main content of the page. Within <main>, I added <article> to indicate that the QR code component standalone content is (it is content that you can share with others, without needing to provide them with other content on the page, in order for them to understand it). Within article I placed <header> to indicate that <img> is the header image of the component. As a sibling of <header>, I used <section>. <section>indicates that a group of content shares the same functionality (in this case, the heading and paragraph). I have to point out here that it might vary from other people. The main idea here is that you try to ask yourself what the purpose and functionality is of specific content. Based on this, you choose the corresponding semantic HTML 5 element.

    3. Try to avoid the use of the style attribute on elements. It is a bad practice because you are polluting your HTML file with CSS. You always should separate these technologies (HTML, CSS, and JavaScript) as much as you can). If you are going to use the style attribute, this makes it harder for other developers to read and edit your code, but also creates semantic problems, because you need to edit your HTML constantly to change the style of an element. Also, you cannot reuse styles such as a class in CSS (unless you do copy paste, which you should not be doing).

    Section: CSS - Feedback

    1. I see you are using background-image: url(''); and justify-content: center; within body element selector. Is there a reason why you are using this? Or did you forget to remove it? If it is not serving a purpose, then I recommend you to remove it. Unnecessary styles always make your stylesheet unnecessarily larger, and developers have more code to read (even code that does not do anything). You even do yourself a favor with this haha.

    2. I see you are using a comment called /* display: inline; */ within .card-header. I think you forgot to remove it.

    Section: CSS - What you did good

    1. You are consistent with the color model you use (in this case, the HSL color model). Some people tend to use color names, hex, HSL, and RGB all at the same time.

    2. You are using relative units (vh, em and %) within CSS, which is always good from a responsive design perspective.

    3. You are using clear and descriptive class names.

    Something else I do want to share with you, which I think is very important, is the one below.

    I noticed in style.css a media query with the media feature max-width. I am not sure whether you made your design for mobile first, but always try to follow the 'mobile first' principle. This means that you should always design (develop) for mobile devices first, then tablets, and then laptops and desktops. If you are going the other way around, you might end up with (a lot of) frustration, because it becomes harder to make your website display correctly on smaller screens (trust me, I have been doing this in the beginning, and you really do not want to be there).

    I hope you can do something with the feedback I gave you. If you have any more questions, feel free to ask me.

    If I made a mistake somewhere in this post, feel free to correct me and keep building awesome things :D.

    Marked as helpful
  • Yari Morcus•500
    @YariMorcus

    Not a problem!

    HTML

    The good thing is that you at least thought about using the semantic HTML5 elements (at least you know that they exist) haha.

    You shouldn't be too worried about your CSS getting too long. It is better to create classes for certain styles (like you already mentioned), than using inline styles. These classes are also called 'utility classes', or at least if you only bound one property to an element using the style attribute.

    As a sidenote: I just noticed that your width: 16rem on the div with class .card could have been placed in the .card class itself.

    What you could have done with your image is using the img selector in your stylesheet, and place width: 100%; height: auto; border-radius: .7em; in it. If you style elements like this, it is also called 'defining global (or generic) styles'. But in normal cases you would have a utility class for border-radius: .7em;, unless you want that border-radius to be applied to all the images.

    CSS

    About your CSS: that sounds familiar haha.

    Mobile-first

    I didn't know that Bootstrap had tools to fix mobile incompatibilities (I do not use it that much). I guess I learned something new here as well haha, thanks!

    I just noticed that I forgot to mention in my first comment that I highly recommend you to follow that principle, if you are building (large) websites/webpages (not just a single component, like you did now).

    If you are not using the mobile-first principle for (small) components, then I am not going to make a problem out of it (or not at all actually, because you have the freedom to make that decision yourself haha). Small components are indeed easier to adjust for smaller screen sizes, but (large) websites/webpages are another story.

    To answer your question "why Mobile first": I found an article that explains the advantages and disadvantages for you: What Are the Advantages and Disadvantages of Mobile-First Design? (opens new tab)

    My advice is to just try it out for your next challenge to get the hang of it. Trust me, it is not as hard as you think it is (especially for small components). With the mobile-first principle, you only use the min-width media feature (within your media query) and not max-width.

    I hope you can do something with my answers, but feel free to ask questions if you still have them.

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