Qr Code Component

Community feedback

• 

  T

  Gwenaël Magnenat•1,270

  @gmagnenatPosted 4 months ago

  Hi, congrats on completing the challenge !

  I noticed a few issues in your solution that you may want to check and fix before moving to the next challenge.

  Does the solution include semantic HTML?

  • It's a detail but this component is probably part of a larger website. So the heading would not be a <h1>. You can add a visually hidden <h1> to your page if you want.
  • A performance tip. You may want to move the title in the head higher just under <meta name="viewport...". there's a great ressource shared on the #-ressources channel on discord I recommand.

  Is it accessible, and what improvements could be made?

  • Currently this solution is easily broken if the default font-size of the browser is changed by the user. I recommand to read this great article -> Why font-size must NEVER be in pixels. You need to use relative unit for everything related to font-size so it can scale correctly and respect the user settings.
  • You want to define a min-height of 100svh (the value you used) instead of a height the reason is : you don't want to limit the height of your element containing any text that can scale. If you force a height at a fixed value, you can have overflow.
  • Fixing a width in pixel on the figure and the img force this image in one size. if the content scale the image doesn't fill its container. Remove the pixel values and add a width:100% on the image. You need to remove also the padding on the image as it breakes the layout, it isn't necessary as the image is contained in the card. Just add a padding to the card itself. remove the heigh of the image also as it needs to be free to scale.
  • You need to remove the position absolute on the attribution. It get placed over the layout when the font-size is increased.

  Does the layout look good on a range of screen sizes? yes

  Is the code well-structured, readable, and reusable?

  • Take a good habit to add a modern css reset at the beginning of your stylesheet. Check out Josh Comeau or Andy Bell. They both have good ones.
  • Your approach for the circles is interesting and good eye catching this!. You need to keep in mind that design isn't always 100% accurate I'm not sure this is supposed to change and it looks more like an opacity issue on the design. In practice you would exchange with the designer if you have a doubt on something. The design should be approach as a guide for the implementation.
  • It's a good practice to take from the beginning to not style the html component directly unless you are doing a global style or a reset. Think of this challenge as a component and not a full page. If you style the h1, it applies to all h1 of your website.

  Does the solution differ considerably from the design?

  • It respects the design intention but needs to be fixed for better accessibility.

  Sorry for the long review but I hope you find something useful in my comments that will help you improve your solution and answers some of your questions.

  Happy Coding !

  Marked as helpful

  0

  ---

  

  P

  cinquante six•140

  @CinquantesixPosted 4 months ago

  Thanks @gmagnenat

  I've made some changes !

  Does the solution include semantic HTML? -Okay for the title. I will look for some explain.

  • I understand, but if FM need a h1 for the report, I will use this instead of, like you say, an other title more logical.

  Is it accessible, and what improvements could be made?

  • thanks I will read it!
  • I know I need to use more min-… max… so I change these everwhere it make sens for me.
  • For the figure, I deleted the width properties but as I didn't take the picture with the background, I can't remove the padding…
  • I changed the z-index for attribution.

  Is the code well-structured, readable, and reusable?

  • I add some reset.
  • Yeah, I know I take this too far, but I really wanted to try and understand bg gradient so I take this like a challenge.
  • I understand. For now, I just don't really know where I need to be more or less specific with my selector. I add a class to isolate the card from maybe other element.

  Thank again for your comments !

  0