Desktop Solution for QR code challenge

Community feedback

• 

  T

  Grace•31,270

  @grace-snowPosted about 1 month ago

  I think you have some html and css misunderstandings going on in this...

  • it's great that you've used a main landmark to wrap the component! But inside it's gone wrong a bit. This card is just an image, h2 and paragraph. I can't stress enough how important it is to really focus on choosing appropriate meaningful elements for the content of a design. It's an essential skill.
  • in the css the biggest issues are the missing css reset and a misunderstanding about units. Definitely add a modern css reset at the start of the styles in every project as already advised.
  • to center the card (as well as min-height 100svh) I'd advise making the body a flex column and using flex properties instead of using grid and place content center as advised. This is because it avoids a potential issue when users enlarge text on mobiles where the content can overflow the screen at the top and left with place content center.
  • it's not wrong but does this card need to be a flex column? Unless you're using gap for the vertical spacings inside the card I can't see any benefit. Block elements already stack vertically by default.
  • don't set margins in percentage or try to use huge margins to build a layout . Again, flex properties on a wrapping element would center the component on the screen.
  • border radius must be a fixed value and not a percentage. Thats important to understand. Each side of the radius would end up a percentage of that length, so it only works for border radius on square or circular elements. You wouldn't use percentage border radius to get rounded corners on rectangular elements.
  • I would set the padding on the card in pixels. The reason I choose pixels over rem is because this is a property I wouldn't want to change if the end user changed their browser or device font size.
  • font size should always be in rem. Note in particular it is dangerous to ever use em for font size because it compounds off the element or parent font size. Just never do it, otherwise it can lead to horrible bugs due to font sizes chaining off each other, one being relative to the other. You'd never want that. Use rem every time.
  • text align center can be set on the component and it would be inherited by the child elements (avoids repetition).
  • don't forget the card component should have a box shadow.
  • this challenge doesn't need a media query. All the component needs is a max width in rem so that it can shrink narrower when it needs to (like on small screens).
  • check the style guide. It gives the expected body font size which you need to convert to rem before use. That is the size on the body, so the size expected on the paragraph. But you have set the paragraph to be the equivalent of 11.2px instead.
  • make sure you understand the difference between padding and margin. Elements like headings and paragraphs shouldn't have padding, only "boxes" in designs as it is for setting inside space. If you want to limit the width of the heading and/or paragraph you'd either do that by setting a max width on them (in ch or rem units) and margin inline auto OR you'd wrap them in an extra div and give that a max-width or inline padding.

  Marked as helpful

  0

  ---

  

  Bhavana Prabhu•80

  @prabhubhavanagPosted about 1 month ago

  @grace-snow Thanks a lot for your valuable feedback. I have updated the css accordingly. Looking forward to learning from you.

  0

  

  T

  Grace•31,270

  @grace-snowPosted about 1 month ago

  @prabhubhavanag it looks like you’re still misunderstanding padding and margin a little. The card should have padding. The children within it should only have vertical margins.

  Move the css reset to the start of your styles instead of making a whole extra network request. It's better for performance.

  Also, it’s important to think of single component demo challenges like this as they would fit in to a real website. We build page templates then build components to be dropped into them. This card is a component that would sit inside the main landmark on a page. It wouldn't be the main landmark.

  0
• 

  P

  Kamran Kiani•1,280

  @kaamiikPosted about 1 month ago

  Hi. Congratulation for doing your first challenge. I have some notes I wanna mention:

  1. Try to use more semantic tags. For your headings, You can use h1-h6 and for your text you can use p. The point of using semantic tags is to make your page more clean, robust and accessible and the screen readers can easily read the content.

  2. On your CSS, use a proper CSS reset. Andy Bell and Josh Comeau is good. You can find them on the net.

  3. Using min-height: 100vh; inside your body let you easily justify your card to the middle of the page and is a standard way.

  4. I'm afraid using percentage for the size unit messes up your layout. Use rem for font size and max-width and em or rem for paddings and margins.

  5. Also on this page If your font sizes and spacing do not get bigger on desktop you do not need media query and with a max-width on your card can simply handle the responsiveness.

  Good luck and keep Going

  Marked as helpful

  0

  ---

  

  Bhavana Prabhu•80

  @prabhubhavanagPosted about 1 month ago

  @kaamiik Thanks a lot for your valuable feedback. I have updated the css accordingly. Looking forward to learning from you.

  1