QR Code

Community feedback

• 

  

  Grace•32,130

  @grace-snow

  Posted over 1 year ago

  This is overflowing my screen - probably because of fixed width but let's get to that in a minute as there are more foundational improvements to make first...

  1. All content should be contained within landmarks. This needs a main wrapping the component and a footer for the attribution.
  2. When building single components think about the context where they would be used. This is not a full Web page but would sit somewhere within a web page, possibly alongside other similar components or QR cards for other websites. That gives you a steer on what the html needs. It's great you've used a h2 not a h1 because this wouldn't be the page title. But is "QR code" a descriptive enough image description? What if there were more of this component on a page? Every image can't be described the same if they go to different websites can it. So make that description richer. Say what it is (QR code) and where it goes (to FrontendMentor.io)
  3. Make sure you update your attribution link eg to your frontend mentor profile page.
  4. get into the habit of including a full modern css reset at the start of the styles in every project. Andy Bell has a good one you can look up and use.
  5. Never limit the height of elements that contain text, including the body. It's fine to use min-height but not height. Min height 100vh allows the element to extend beyond the minimum set if and when it needs to.
  6. Same goes for the component. You must remove the height from that. All that can cause is overflow bugs when a user has a different text size, font family or line height; or when an editor writes more words in a card. There is no need for a min height either in this case. Let the browser do it's job and decide what height is needed based on the content of the card.
  7. The card must not have a width either. All it needs is a single max width in rem. That allows it to fill the available space up until that width, but also allows it to shrink narrower when viewed on a smaller screen. It should be in rem so the layout works for all users including those who have a different text size.
  8. don't style on IDs thats not what they are for. In fact you must remove any IDS from this components html! IDs must always be unique on a page and as already mentioned its likely that card components would repeat. (In future challenges you'll learn how to make IDs unique but don't worry about that now).
  9. You can place text align center on the card component in this instead of repeating it on the card child elements. It's a property that would be inherited.

  ---

  FYI @LuciaVerde and @nonoza you may like to read the above feedback on this challenge as you also reviewed it 🙂

  30

  Marked as helpful
• 

  Pretty Kunene•690

  @nonoza

  Posted over 1 year ago

  Heya!!

  First of all, congrats in completing the project. You did very well.

  To answer your Questions (My opinion):

  • Yes, I also use it. We all have different says to solve the problem . So your styling might not be the same as mine but aslong what you know what the property does and know how will solve a an issue , then you are good.
  • Suggestion : I would give a margin bottom to a container to push the attribute down.

  Otherwise good job.

  10
• 

  Lucia Verde•60

  @LuciaVerde

  Posted over 1 year ago

  I don't have much to say, I think it's a very good project. I can only suggest you to rename the css file to style.css because "index" is normally used for html file. Also, instead of using fixed values for the height and width of the .white-container consider using relative measurement units for a more responsive design. Keep going, you got this!

  10