QR code component using HTML, CSS, and React + Vite

Community feedback

• 

  Sizwe Mashele•170

  @seezmashelePosted over 1 year ago

  Hi Celi 👋

  Great job on your first Frontend challenge! It's awesome that your code is neat and uses accurate HTML tags for text and headings etc.

  I noticed the position of the card moves up and down based on the width of the page which some users might not expect, but hey, we're allowed to have a little fun here 😄.

  You could add a shadow to the card as well if you want to match the design a little closer.

  Other than that, it's big thumbs ups for diving in 🚀

  Marked as helpful

  1

  ---

  

  Celi•10

  @CelesTech03Posted over 1 year ago

  Hi @seezmashele, thank you for taking the time to provide feedback! I'll implement the changes.

  1
• 

  T

  Grace•31,230

  @grace-snowPosted over 1 year ago

  Hi

  Before I feed back, is there a reason you've used React for this? There are some foundational issues in html and css that are usually ironed out when learning them before moving ahead to a javascript framework. And part of the challenge as a front end developer is choosing the most appropriate tool for the job

  Marked as helpful

  0

  ---

  

  Celi•10

  @CelesTech03Posted over 1 year ago

  Hey @grace-snow, there was no particular reason. I realized afterward that it was a bit overkill considering the requirements of the challenge.

  0

  

  T

  Grace•31,230

  @grace-snowPosted over 1 year ago

  Ok definitely better to do this one in vanilla @CelesTech03

  I hope these are helpful pointers

  • all page content must be contained by landmarks. The card should be inside a main and attribution a footer
  • if you're going to use a framework at least create reusable components which is the whole point of it
  • the heading level should be higher
  • the alt text needs to clearly say what this image is (QR Code) and where it goes (for the frontend mentor website). Alt should never include words like "image" because it already has an image role
  • it is more performant to load fonts in the head than import in css. That leads to a delayed server call
  • youre missing a min height on the outer wrapper that will correctly center the component on all screen sizes. By default the body will only be as tall as it's contents so give it a min height 100dvh if you want it to fill the screen
  • don't use margin 10% as that gives unpredictable results (it will change a lot for different users). Instead, this component should have a little margin with a fixed value on all sides so it can never hit screen edges.
  • as with all projects, you should be using a modern css reset at the start of the styles. Andy Bell has a good post about this on his website with one you can use
  • the img and component should not have a fixed width in this. The img should be width 100% and the component should only have a max width in rem
  • place padding on the component. Actually there is no reason for an extra div wrapping the content of the card. That's extra html and splitting the styles up for no reason. Always try to keep the structure as simple as it can be
  • Never ever write font size in px https://fedmentor.dev/posts/font-size-px/

  0