QRCode using CSS Flexbox

Community feedback

• 

  T

  Grace•31,270

  @grace-snowPosted almost 3 years ago

  Hi Anna, Well done on your first challenge! This is the time to build those foundational good practices that will stay with you long term.

  Feedback for html

  • use landmark elements. That means main for the container and footer for attribution
  • I wouldn’t use article for this component, but you can if you want.
  • more important is to use a heading on this. The accessibility report will warn you this should be a h1 because every page has to have one, but as this isn’t a full web page, I’d probably use h2 for this component.
  • images have to have an alt attribute. In this case the image is really important, so the alt needs to contain a description like “QR code”
  • there’s nothing wrong with wrapping the img in a div, but it shouldn’t be necessary. You can set images to be display block in css

  I’ve not got time to feedback on css right now but will do soon

  Marked as helpful

  0

  ---

  

  Anna•130

  @Anq92Posted almost 3 years ago

  @grace-snow Thank you for the feedback, it's really clear and helpful!

  0

  

  T

  Grace•31,270

  @grace-snowPosted almost 3 years ago

  Here are some notes on changes I'd recommend to css and why. I made them quickly in browser dev tools, so please excuse th formatting

  html {
    font-size: 62.5%;
    note: Strongly recommend against doing this. Resizing your html base font size can cause huge accessibility issues for people who rely on different font/zoom settings. There's no need to make 1rem equal 10px, let it be the default.
  }
  
  body {
    margin: 0;
    note: include a small css reset at the start of every stylesheet eg Reset by Josh Comeau or Andy Bell. This will make all browsers treat elements the same and remove things like margin from the body.;
  }
  
  .box {
    /* height: 500px; */
    /* width: 320px; */
    /* margin: 0 auto; */
    max-width: 30rem;
    margin: 1rem;
    padding: 1.8rem;
    note: try to use rem weherever you're tempted to use px. It doesn't have to be exact for these challenges but is a good habit to get into as early as possible. Tweak in browser dev tools until it looks about right;
    note: don't add height to elements unless you actually need to. Let the component grow if it needs to.
    note: max-width is better than width for this because it allows the component to shrink if needed on smaller screens without causing overflow/horizontal scroll.
    note: margin 1 rem is to stop the component hitting screen edges on small screens.
  }
  
  .img-setting {
    /* height: 287px; */
    /* width: 287px; */
    /* margin: 17px; */
    display: block;
    max-width: 100%;
    note: no need for width or height on this
  }
  
  .text-container {
    /* position: relative; */
    /* margin: 0px 0px; */
    /* width: 250px; */
  }
  
  p.main-text {
    /* margin: 5px 0px 10px 0px; */
    note: why is this css selector more specific? Keep specificity low, to single classes if possible
    note: must be a heading;
  }
  
  .main-text {
    /* letter-spacing: 0px; */
    margin: 1.6rem 0;
    note: letter spacing should be unitless, not px. When letter spacing is 0 that's already the default so this line can be removed.;
  }
  
  p {
    margin: 1.6rem 0;
  }
  
  

  Marked as helpful

  0
• 

  MaryF•350

  @JanselinPosted almost 3 years ago

  Hey there! Congrats on your solution! you did great job! I think you did very well with the use of <div>. Here some tips!

  🔹I would recommend you to use a <article> tag for the div class="box"

  🔹 you could replace <div class="container"> for a <main > tag.

  🔹 <p class="main-text"> should be a <h1>

  Just for a better html structure.

  Happy coding 🤗

  Marked as helpful

  0

  ---

  

  Anna•130

  @Anq92Posted almost 3 years ago

  @Janselin Thank you very much, I'll use the tips :)

  0
• 

  Hugo Padilla•210

  @HugoPadillaPosted almost 3 years ago

  Saludos! buen trabajo.

  0