Skip to content
Submitted about 2 years ago

QR code, CSS fundamentals

Carl Fremault•20
@carlfremault
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

  • Travolgi 🍕•31,400
    @denieldenPosted about 2 years ago

    Hi Carl, congratulations on completing the challenge, great job! 😁

    Some little tips for optimizing your code:

    • add main tag and wrap the card for improve the Accessibility
    • also you can use article tag instead of a simple div to the container card for improve the Accessibility
    • use h1 for the title of card and not a simple p
    • remove all margin from .container
    • use flexbox to the body to center the card. Read here -> best flex guide
    • after, add min-height: 100vh to body because Flexbox aligns child items to the size of the parent container
    • instead of using px use relative units of measurement like rem -> read here

    Hope this help! Happy coding 😉

    Marked as helpful
    1
    Carl Fremault•20
    @carlfremaultPosted about 2 years ago

    Hi @denielden, many thanks for your helpful feedback!

    1
    Travolgi 🍕•31,400
    @denieldenPosted about 2 years ago

    @carlfremault You are welcome and keep it up :)

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

    Hi Carl Fremault,

    Congratulation on completing your first frontend mentor challenge. Your solution looks great. I have some suggestions regarding your solution:

    • You should use <main> for the card and <footer> for the attribution. HTML5 landmark elements are used to improve navigation experience on your site for users of assistive technology.
    • You can use a header for class="card-title, in this challenge, you can use <h1>.
    • 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.
    • In order to center the card on the middle of the page , you can use the flexbox properties and min-height: 100vh for the <body> add a little padding to the body that way it stops the card from hitting the edges of the browser you can remove the large margin
    • width: 300px an explicit width is not a good way to have responsive layout . Consider using max-width to the card in rem .
    • Consider using rem and em units as they are flexible, specially for font size better to use rem. If your web content font sizes are set in absolute units, such as pixels, the user will not be able to re-size the text or control the font size based on their needs. Relative units “stretch” according to the screen size and/or user’s preferred font size, and work on a large range of devices
    • Remember a css reset on every project. That will do things like set the images to display block and make all browsers display elements the same.

    Overall, Excellent work! Hopefully this feedback helps.

    Marked as helpful
    1
    Carl Fremault•20
    @carlfremaultPosted about 2 years ago

    Hi @PhoenixDev22 , thank you for your time and feedback, much appreciated ! With regards to CSS reset, I've been reading up a little and while I certainly understand the necessity, the provided examples left and right on the web (e.g. https://elad2412.github.io/the-new-css-reset/) seem a bit overkill for such a small project ? Or is it just a question of good practice and getting used to it, no matter the project size ?

    (at the moment I just added 'box-sizing: border box' on top of the zeroing of margins and padding implemented before ...)

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

    @carlfremault

    Glad to help.

    I agree that it would be a bit overkill for a one component, but in real life you would be working with big projects, So it is worth getting used to it from the beginning and implement it. It's your own choice as developer to chose what right for your project.

    Thanks for sharing your approach. Happy coding!

    Marked as helpful
    1
    Carl Fremault•20
    @carlfremaultPosted about 2 years ago

    @PhoenixDev22 Understood! Thanks a lot!

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

    👾Hello @carlfremault, congratulations for your new solution!

    Your component is perfect, but is not responsive yet, this is due the fixed width you've applied to the container.

    Look both width and max-width the main difference between these properties is that the first(width) is fixed and the second(max-width) is flexible, for example, a component with width: 300px will not grow or shrink because the size will be ever the same, but a container with max-width: 300px or min-width: 300px can grow or contract depending of the property you've set for the container. So if you want a responsive block element, never use width choose or min-width or max-width.

    REPLACE width: 300px; WITH max-width: 300px; to make it responsive

    .container {
    background-color: hsl(0, 0%, 100%);
    border-radius: 5%;
    margin: 80px auto;
    padding-bottom: 20px;
    position: relative;
    text-align: center;
    max-width: 300px;
}

    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.

    👋 I hope this helps you and happy coding!

    Marked as helpful
    1
    Carl Fremault•20
    @carlfremaultPosted about 2 years ago

    Hi @correlucas , thanks a lot for your feedback, much appreciated !

    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