QR Code Component using simple CSS

Community feedback

• 

  PhoenixDev22•16,830

  @PhoenixDev22

  Posted almost 3 years ago

  Hi Isaac Vono,

  Congratulation on completing this frontend mentor challenge. Your solution looks great. I have some suggestions regarding your solution:

  • In my opinion, the image is an important content. The alternate text is needed on this image. The alternate text should indicate where the Qr code navigate the user : like QR code to frontend mentor not describes the image. The alternate text should not be hyphenated, it should be human readable.

  • Add min-height: 100vh to the body that let the body grows taller if the content outgrows the visible page instead.

  • width: 256px; an explicit width is not a good way to have responsive layout . Consider using max-width to the card in rem .

  • Adding max-width on the body tag to prevent layout from stretching. If you try to zoom out on your browser , you'll see that the layout stretches, adding max-width will prevent that. Personally, I don’t restrict the width or height of the body element. If I need to restrict the width I use a container div with a max-width on it.

  • Remember a css reset on every project. That will do things like set the images to display block and make all browsers display elements the same.

  Overall, Excellent work! Hopefully this feedback helps.

  00

  Marked as helpful
• 

  Ahmed Kamel•1,190

  @hmadamk

  Posted almost 3 years ago

  first of all you should never use fixed height unless really neccessary, so to center your main card you should use body{ min-height:100vh } and you don't need to add the width on the body secondly you should import the font from google fonts the link is on the readme you check the rquired weights and add the generated link on the top of your html code; lastly to control your text width you should use the ch unit wich is the width of the number zero like that p{ width:32ch }

  00

  Marked as helpful
• 

  Sathya D•310

  @satzzzzz07

  Posted almost 3 years ago

  Hey there, Great work on your first solution! Design looks good. There are few suggestions:

  1. Try to use the fonts mentioned in the style-guide file. You can import the fonts from google fonts(https://fonts.google.com/). Its a simple process, choose the font, size and then import them into your css or html files. You can use the font style using font-family css property.

  2. Centering the Element - For Centering the element in the page you can wrap the element in a div. and use flexbox more about flexbox.

  Aside these, excellent work again and happy coding :) 👍

  00

  Marked as helpful
• 

  David•7,960

  @DavidMorgade

  Posted almost 3 years ago

  Hey Isaac, congrats on finishing the challenge and welcome to the community, you did a pretty good job for a first challenge!

  It seems that you had some trouble centering your component, I would recommend you to remove the fixed width and height from your body cause that is causing some strange behaviour on your layout.

  Apart from that, since you already have display: grid on your body, you just need to use the property place-content: center (this will center your item inside the grid to the center), and also set the body height to the full screen with height: 100vh;.

  With just this 2 properties and removing the fixed width and height, you should get your component on the middle of the screen!

  Hope my feedback helps you, if you have any questions, don't hesitate to ask!

  00

  Marked as helpful