Submitted over 1 year ago

HTML CSS

chiozoadiro• 150

@ejikemechiozoadiro

Preview site View code

This is a solution for...

QR code component

HTML
CSS

1newbie

View challenge

Design comparison

SolutionDesign

Solution retrospective

This is my first challenge on Frontend Mentor I'd really appreciate feedback on the challenge

Community feedback

Yari Morcus• 480

@YariMorcus

Posted over 1 year ago

Hi @chiozoadiro,

First of all, congratulations on completing your first challenge! I took a look at your design, and it looks pretty good.

However, I do have some feedback I want to share with you so that you can become even a better developer than you already are.

My feedback is divided into sections to make it more clear.

Mobile

Your component should be a little wider. If you do this, your text will align better according to the design both on mobile as desktop. Besides that, the width of your component will be both the same on mobile and desktop (you do not have to adjust anything specific for desktop).

Tips

You can center any component horizontally and vertically by doing the following (assuming that the page only contains a component). Place min-height: 100vh;, display: grid; and place-items: center; on <body>. If you do this, you do need to remove the div with id forest-ext-shadow-host (just as a sidenote: I do not see any CSS attached to this id, so I think you might forgot to remove it?).
Always place the width and height attributes on <img>, and use the pixel values of the width and height of your image (the size you are going to use). This gives as a result that your page will load faster, the page won't jump when suddenly an image loads in (very frustrating for users), and your page will act smoother.
I saw you have been using rem values for your widths, margins, paddings and font-sizes, and used font-size: 62.5% on <html>. If you want to make it easier for yourself to calculate your em and rem values, then you should replace 62.5% with 10px. If you want to use em and rem values, the only thing you have to do is font-size (or width size) / 10 = your em/rem value. This will make it a lot easier for you instead of using a percentage on your root element (<html>).

Things you did good (may also be said)

Even though the width of your component should have been a little wider, you still did a great job on making it look as close as possible to the design.
You placed the specific part of your content within <title> upfront (I hope that I am saying this correctly haha).
You implemented BEM correctly.
You have been using the color models consistently (HSL in this case).

I hope you can do something with the feedback and tips 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 😄

Marked as helpful

chiozoadiro• 150

@ejikemechiozoadiro

Posted over 1 year ago

@YariMorcus Thanks a lot

javascriptooo• 330

@javascriptooo

Posted over 1 year ago

Given the accessibility issues -- you need to include at least the <main> landmark.

The <main> landmark tag can be inserted around your content inside the <body>. (Looks like you forgot to place the opening <body> tag as well.) Here is some documentation

Adding the <main> landmark will remove 2 accessibility issues! :)

Marked as helpful

chiozoadiro• 150

@ejikemechiozoadiro

Posted over 1 year ago

@javascriptooo Thank you so much for the feedback

Lucas 👾• 104,580

@correlucas

Posted over 1 year ago

👾Hello Chiozoadiro, Welcome to the Frontend Mentor community and congratulations for your first challenge solution!

Your solution is already really good, you've match all design elements and the component is already done. I've some tips for you about the card alignment and how you can clean a bit your code.

Here's my tips:

1.Wrap all the container components with <main> instead of <div> to keep a better semantic.

2.Work the html structure and clean your code. Note that you've only one img, h1 and p. For this reason you'll not need any class to select this elements, you can go with the element itself. See a clean html structure for this challenge below:

<body>
<main>
<img>
<h1></h1>
<p></p>
</main>
</body>

3.Align the card component to the screen center using display: flex; its alignment properties and min-height: 100vh; and you'll se your component fully aligned, code below:

body {
    min-height: 100vh;
    display: flex;
    text-align: center;
    background: #d6e2f0;
    font-family: Outfit,Arial,Helvetica,sans-serif;
    align-items: center;
    justify-content: center;
}

Hope it helps, happy coding!

Keep it up.

chiozoadiro• 150

@ejikemechiozoadiro

Posted over 1 year ago

@correlucas Thanks a lot for your feedback

Lucas 👾• 104,580

@correlucas

Posted over 1 year ago

@ejikemechiozoadiro Happy to hear that, keep it up Chio!

Please log in to post a comment

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