Skip to content
Submitted about 2 years ago

flexbox

Arturo Gonzalez•30
@Arthur-Gnz
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

Could you give me feedback? (Anything is accepted)

Community feedback

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

    👾Hi Arturo Gonzalez, congrats on completing this challenge!

    You’ve done really good work here putting everything together, I’ve some suggestions you can consider applying to your code to improve the html markup/semantic:

    The colors you’ve used are a little bit different from the original colors.When you download the project files there’s a file called style-guide.md where you can find information such as font-family, hsl color codes, device sizes and the font-size for the headings.

    The html structure is fine and works, but you can reduce at least 20% of your code cleaning the unnecessary elements, you start cleaning it 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 Image" >
 <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>

    ✌️ I hope this helps you and happy coding!

    Marked as helpful
    0
  • Akram Adjab•380
    @akramAdjabPosted about 2 years ago

    Hello, I happy for you because you complete this challenge. But there are some mistakes when I saw your code

    1. Page should contain <h1>. In this challenge, you may use <h1>, because every page should always contain <h1> to improve your SEO.
    2. In the wrapper, there is no need to center it using position: absolute, because you declare in the body tag display: flex just give it align-items: center and the component will be perfectly centered.
    3. Also in justify-content: top the top value doesn't exist, instead, you can use flex-start and this is the default value, and you don't have to use it because your content is already starting from the top.
    4. Don't use <br> in this challenge, let the content fill the entire width.
    5. Don't give the wrapper a height, always set the height: auto, because you broke the aspect-ratio for the image.

    I hope my feedback is helpful to you! You also check my code for this challenge and compare it to yours!

    Marked as helpful
    0
  • Alex Martos P.•280
    @AlexMartosPPosted about 2 years ago

    Good job! I see that you tried using the font from the design. To use it you have to.

    1. Get the Google fonts via the link here: https://fonts.google.com/specimen/Outfit?query=outfit
    2. Get the font family by font-family: 'Outfit', sans-serif (sans-serif is the fallback)

    Other thing you can think about is using padding/margin instead of <br> and "font-weight" instead of <strong>

    But again, good job!

    Marked as helpful
    0
  • Milos Djordjevic•990
    @milosshomyPosted about 2 years ago

    You can use min-height instead of height on div with a class wrapper. That way the text won't be outside of the wrapper. Good job, keep coding! :D

    Marked as helpful
    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