Skip to content
Submitted almost 2 years ago

QR code component

Musicalaudio•160
@Musicalaudio
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

Hey, thanks to anyone reviewing my solution.

One of the main things I was wondering about was how to decide on a border radius of an inner element (in this case the image), once a border radius for the outer section (in this case the card) is already decided, or vice-versa.

Another thing I would appreciate feedback with is whether I'm using the correct units for the correct properties. I know font-size should usually be rem, and I think padding should be rem too. But I'm not too sure if there are any hard rules about what units should be used for any other properties.

Thanks again.

Community feedback

  • Vanza Setia•27,775
    @vanzasetiaPosted almost 2 years ago

    Hi, Musicalaudio!

    In this case, you need to set the same amount of border-radius to the card and the image. There is no way to apply the card's border-radius to the image automatically. So, set border-radius to the image and the card.

    I have some more suggestions to improve this solution.

    • Remove the <picture> element. You only need to use it when the image is having different versions for different screen sizes. Learn more about the use cases — <picture>: The Picture element - HTML: HyperText Markup Language | MDN
    • Make the <main> element as the card. This way, you can remove the <article> element.
    • Remove the <section> element. Then, control the card's padding instead. After that, use margin to give some spacing between the heading and the paragraph.
    • Put class="attribution" on the <footer> element. This way, you can remove the extra <div>.
    • Alternative text should not start with "image of". It is already an <img> element so there is no need to repeat that information in the alternative text.
    • Don't change the <html> or the :root font size. It can cause huge accessibility implications for those users with different font sizes or zoom requirements. You can learn more about the reasons:
    • Never use px unit for font sizes. Use rem or em instead. Relative units such as rem and em can adapt when the users change the browser's font size setting.

    I hope this helps. Happy holidays!

    Marked as helpful
    1
    Musicalaudio•160
    @MusicalaudioPosted almost 2 years ago

    Thanks for the feedback @vanzasetia, the only thing I have a question about is the use of the article tag.

    I understand that I could get rid of an element if I moved the card to the main tag, but I thought that by using the article tag, it would communicate the semantics of a card better. Since a card is self-contained and can be understood without any outside context, which is what an article tag is meant to represent, I thought that this would be more appropriate? Correct me if I'm wrong.

    Thanks again, and happy holidays to you too!

    0
    Vanza Setia•27,775
    @vanzasetiaPosted almost 2 years ago

    @Musicalaudio

    You are welcome!

    <article> and <section> don't do anything by themselves. Those elements only have meaning when they are labeled by using aria-label or aria-labelledby. So, those are the same as <div>.

    Reference: WebAIM: HTML Semantics and Accessibility Cheat Sheet

    In this case, having <article> will not make the HTML more semantic. So, by removing the <article> element, you keep the HTML as simple as possible.

    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