QR-Code - Responsive design with flex box

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted almost 3 years ago

  Hey, nice work on this one. I see that you somehow went on another approach on this one, it's fine but I really recommend that when you tackle other challenges, following the design itself should be the best ui approach :>

  Here are some other suggestions besides Deniel helpful suggestion and don't mind me reiterating some already suggested ones:

  • Using main tag on the .container instead of div would make the site more accessible. When creating websites, you should use main tag to nest the main-content of that webpage, on this case, the whole content should be inside the main.
  • To center the contents on your solution, you don't need to use margin on the .container since it is not really consistent. What you can do is that, add these stylings on the .container:

  justify-content: center;
  min-height: 100vh;
  

  This way, the item will be centered properly and dynamically.

  • The margin on the .card could be removed since its parent ( .container) is already making it centered.
  • For the img, you could just use alt="" and aria-hidden="true" on it since the image doesn't really depict anything visually and could just be left empty so that screen reader won't read it.
  • Also when you use img or svg , you don't use words that relates to graphic like "image"` since those elements are already graphics.
  • For the bold text, you can use h1 on it for now. Remember that a webpage needs to have an h1 and you can replace the p tag with it on this.
  • When wrapping up text content, use meaningful elements like a p tag, change your span into using p tag.

  Aside from those, great job again on this one^^

  Marked as helpful

  1
• 

  Travolgi 🍕•31,400

  @denieldenPosted almost 3 years ago

  Hello Hersi, great job! Congratulations on completing the challenge.

  I have a few advice for you:

  • add main tag and wrap the card for Accessibility
  • remove margin, max-width and min-width properties from container class
  • remove all margin from card class
  • use justify-content: center; to the container class for center the card. Read here -> flex guide
  • add heigth: 100vh to container class because Flexbox aligns to the size of the parent container.

  Overall you did well :)

  Hope this help and happy coding!

  Marked as helpful

  1

  ---

  

  Raymart Pamplona•16,100

  @pikapikamartPosted almost 3 years ago

  @denielden Hey, really nice feedback on this challenge. One thing to change though, instead of height: 100vh always use min-height: 100vh so that the element won't have a fixed height. That's it and really nice feedback again^^

  Marked as helpful

  1

  

  Travolgi 🍕•31,400

  @denieldenPosted almost 3 years ago

  @pikapikamart Thank you so much! You're right :)

  Marked as helpful

  1
• 

  2div•170

  @2divPosted almost 3 years ago

  Thank you both you @Deniel & @Raymart , your comment and feedback was very very helpful.

  @Raymart thank for your deep explaining it was really helped me a lot since im in the beginning in this field.

  I have revised my code and design and applied what learned from both you.

  I am really amazed how my code is less and everything is good.

  Only one thing on mobile display not okay, the card look too small , i could not find out why ? any idea about it?

  1

  ---

  

  Raymart Pamplona•16,100

  @pikapikamartPosted almost 3 years ago

  @2div Hey, great that you find it useful^^

  I think you just need to adjust the height and width of the .card selector on this. Also, I don't know if you coded it to be smaller but check if you are zoomed in or zoomed out when coding. Looks great by the way:>

  Marked as helpful

  1