QR code component webpage

Community feedback

• 

  T

  Grace•31,270

  @grace-snowPosted 3 months ago

  Im afraid this has been made far more complicated than it needs to be. Definitely don't move on until you've had feedback and refactored this. (I'll try to list everything out for you tomorrow as its very late here now and I need to sleep!😂)

  1

  ---

  

  T

  Grace•31,270

  @grace-snowPosted 3 months ago

  Ok here are things I notice that you should change in this solution. I hope it's helpful.

  • remove all these divs! This is a single card component. You only need one div with an image, heading and paragraph inside. There is no reason at all to wrap everything in a div like you are doing now. It makes everything so much harder to read, to style, everything. Break that habit ASAP.
  • all content should be contained within landmarks. So outside of that component, wrap it in a main. Every page you ever build should have one main landmark. The attribution should be in a footer landmark (as a sibling of main).
  • When we build single components like this, it's important to think through the context of where/how it will be used in a real site. This is a card. It's likely there may be multiple of them on a page at any one time. That tells you that the alt text you've used on the image isn't descriptive enough. It needs to say what the image is (QR code) and where it goes (to FrontendMentor.io).
  • make sure your attribution link a tually goes somewhere! Like your frontend mentor profile or your github profile.
  • it's better for performance to link fonts in the html head instead of css imports.
  • get into the habit of including a full modern css reset at the start of the styles in every project. Andy Bell or Josh Comeau both have good ones you can look up and use.
  • to center a component on a demo page like this, make the body min-height 100svh (by default it's only as tall as it's content) and use flex column properties on it that will center its child elements.
  • never limit the height of elements that contain text. By setting an explicit height on this you afe breaking it for some users. Think about what height 100vh means. On a mobile landscape for example that is tiny and content would have to overflow. Similarly, what happens if the font size or content is changed at some point. As soon as you limit the height of these components, stuff will break. Whereas min-height (only when you need it, which isnt often) would allow elements to grow as needed.
  • don't set empty border properties for no reason. Once you've simplified the html you will be able to delete loads of these style declarations. Make sure every single property is only there is it needs to be there. Always know the reason you're adding something.
  • The component must also not have a width, and especially not in px. Instead, give it a max width in rem. That will allow the component to grow up to the limit set, but also allow it to get narrower when it needs to like on smaller screens. The reason we use rem for this is so that the layout adapts correctly even when people change their default text size.
  • there's no need for aspect ratio on the card. That can onky cause problems for the browser as it won't know what to do if the component is viewed on a small screen or by someone with a different text size (both cases where that aspect ratio may need to adjust). Let the browser do it's job and decide what height and width are needed.
  • remove huge margins. I've already covered how to center the component on the screen above. As soon as you're tempted to use huge margins or paddings to create a layout pause and rethink — it usually signals an error in approach.
  • there's no need for any grid or flex on the card. The only reason would be to use the gap property for vertical spaces but that can also be done with margins.
  • the img only needs a border radius, plus display block and max width 100%, both of which would already be included in the css reset. Remove all other styles, they're unnecessary.
  • Font size must never be in px. Use rem instead.
  • remove the media query. There is no need for any media query in this challenge and once you've made the changes above you'll see you don't need it. For future challenges though, this isn't how you should approach using media queries. We build mobile first in almost all cases — so mobile styles should be the default. Then we add one or more min-width media queries defined in rem or em not px. There is a post on my FEDmentor.dev site linked above about how to make sites responsive that may be worth you reading I think.

  Marked as helpful

  0
• 

  Ayomide00712•20

  @Ayomide00712Posted 3 months ago

  You could have made you container to have a 100vh size

  0

  ---

  

  T

  Grace•31,270

  @grace-snowPosted 3 months ago

  @Ayomide00712 I'm afraid that's not good advice. Did you mean to advise on using a min-height 100svh for the body? That's what's needed. Limiting height would break the solution for lots of people.

  0