QR-code- component

Community feedback

• 

  

  Grace•32,130

  @grace-snowPosted 7 months ago

  It's important not to move on to new challenges until you've read feedback and refactored first. I can see bigger problems on the blog card challenge but came back to feedback on this one first because the problems will only get bigger and bigger with each new challenge if you don't take time to fix these things before moving on. I hope this is helpful.

  • all content should be contained within landmarks. Although this is just a single component demo, get into the habit of including at least a main landmark to each page.
  • the image is the most important content on the page. That means it needs a proper alt description. The alt text on this needs to say what the image is (QR code) and where it goes (to FrontendMentor.io).
  • is there a reason you've chosen h3 as the heading level in this? (Not necessarily a problem, I just want to check whether it's an intentional choice).
  • always include a full modern css reset at the start of the styles in every project. What you have at the moment is not a reset. Look them up. Andy Bell or Josh Comeau both have good ones you can look up and use.
  • there's no need for the container div in this. All of the styles for centering the component on the screen can go on the body or main.
  • don't limit the height of elements that contain text, including the body / container. Use min-height 100svh instead of height. That tells the browser it is allowed to go taller than that when it needs to.
  • there is no benefit at all to making this card component into a flex column. All of the child elements would stack vertically by default anyway. Keep styles simple.
  • this card must not have an explicit width. Use max-width in rem instead. That let's it go narrower when it needs to like on small screens.
  • the card must not have a height at all. That's important to grasp. It is the browsers job to decide what height is needed based on the content inside. Never limit height like that, all it can do is cause overflow bugs.
  • the card should have padding on all sides.
  • the image must not have an explicit width and height either. All it needs for sizing is what's already included in the css reset - display block and max-width 100%. You can give it an aspect-ratio of 1 if you like for a little performance boost so the browser saves the space for it while the image loads.
  • the description shouldn't have a height or vertical padding. Make sure you understand the difference between padding and margin actually. https://fedmentor.dev/posts/padding-margin/ If the goal is to ensure the line breaks happen at the same time as the design you'd do that with a max width in rem and margin inline auto.
  • font size shouldn't ever be in px. Use rem. https://fedmentor.dev/posts/font-size-px/
  • it's not wrong but usually line height is unitless like 1.5.
  • letter spacing should be in em or rem not px.

  Marked as helpful

  3
• 

  P

  Michał•290

  @miedzygalaktycznygitPosted 7 months ago

  You need to make a bit smaller space between qr code and text container and it will be perfect. I also saw that if webside is to short vertically to fit content it end up getting cutted. My advice to this problem is setting minimal height to the size of your content

  0