Skip to content
Submitted about 2 years ago

QR-code component

Cintia•100
@ciisiq
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

Solution retrospective

Hi guys, First I want to say thank you to everyone that stop here and give feedback you don't know how this can help newbies like me :)

I need to improve my skills with accessibility, anyone can send me something? I tried to use all the feedback I had in my last one to improve this one I don't see the difference between the mobile and desktop versions so I did just the mobile, is it a problem?

Thank you!

Community feedback

  • Lucas 👾•104,400
    @correlucasPosted about 2 years ago

    👾Hi Cintia, congratulations on your solution!👋 Welcome to the Frontend Mentor Coding Community!

    Great solution and a great start! From what I saw you’re on the right track. I’ve few suggestions for you that you can consider adding to your code:

    👾 1.The main heading has the tag <h3>, in this case, you should replace it with <h1> since this heading is the main title on this page. Remember that every page should have one <h1> to declare which is the most important title and that you should follow the hierarchy using the heading sequence (h1, h2, h3, h4, h5) and never jump a level.

    👾 2.Your HTML code is not optimized yet, since it's too long and has some unnecessary elements. To make it clean you start by removing some unnecessary <div>. For this solution you wrap everything inside a single block of content using <div> or <main> (better option for accessibility) and put inside the whole content <img> / <h1> and <p>.

    <body>
<main>
<img src="./images/image-qr-code.png" alt="QR Code Frontend Mentor" >
 <h1>Improve your front-end skills by building projects</h1>
<p>Scan the QR code to visit Frontend Mentor and take your coding skills to the next level</p>
</main>
</body>

    👾 3.To reduce the CSS you can use the direct selector for each element instead of using class this way you have a code even cleaner, for example, you can select everything using the direct selector for (img, h1, and p, main).

    👾 4.Note that there's no need to use height here, because since you set a height for an element, this means that this element will grow until a certain point and after that the inner content (as texts or images) will start to pop out the element due its fixed height, so isn't necessary to set the height the container height comes from the elements, its paddings and height.

    5.The image is not responsive yet, a quick way to make any image responsive and respecting the container size is to add display: block and max-width: 100% to the <img> selector.

    img {
    display: block;
    max-width: 100%;
}

    👾 6.Fix the component responsiveness, its not working yet and this is due the fixed width you've applied to the container. The difference between width and max-width is that the first(width) is fixed and the second(max-width) is flexible and make the element shrink when the screen starts to scale down. So if you want a responsive block element, never use width choose or min-width or max-width.

    Here's my solution for this challenge if you wants to see how I build it: https://www.frontendmentor.io/solutions/qr-code-component-vanilla-cs-js-darklight-mode-nS2aOYYsJR

    ✌️ I hope this helps you and happy coding!

    Marked as helpful
    0
    Cintia•100
    @ciisiqPosted about 2 years ago

    @correlucas wow thank you a lot, when I sent the solution I saw the website complain about the H1 I didn't understand but now makes a lot of sense!! Thank you so much for your time I will improve my code =) I will check your solution!!

    1
    Lucas 👾•104,400
    @correlucasPosted about 2 years ago

    @ciisiq You're welcome Cintia! I'm looking forward to see your next challenges here. Keep it up =)

    0
  • PhoenixDev22•16,950
    @PhoenixDev22Posted about 2 years ago

    Hi Cintia,

    Congratulation on completing another frontend mentor challenge.

    Well done ! You already received a helpful feedback which is nice to see , just going to add some suggestions as well if you don't mind:

    • Consider using min-height: 100vh instead of height: 100vh to the body , that let the body grows taller if the content of the page outgrows the visible page.
    • height: 28rem; It's not recommended to set fixed height to component, you almost never want to set it. let the content of the component define the height
    • Remember a modern css reset on every project that make all browsers display elements the same.

    Adding rel="noopener" or rel="noreferrer" totarget="_blank"links. When you link to a page on another site using target=”_blank” attribute, you can expose your site to performance and security issues.

    Hopefully this feedback helps.

    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