responsive card layout

Community feedback

• 

  T

  Gwenaël Magnenat•1,290

  @gmagnenatPosted 5 months ago

  Hello, congrats on completing the challenge !

  I had a check on your preview and your repo. I have a few comments I can share that may help you improve your solution and your future challenges.

  This challenge may seem easy but has a lot of things packed to identify gaps in html and css foundations necessary for a better web accessible to all.

  Does the solution include semantic HTML?

  • add the title at the top of your head just under the <meta name="viewport".. this is recommanded for performance.
  • your solution is missing a <main> landmark.
  • keep the html simple. I don't think you need to wrap that image in an extra div.
  • this is a preview card and I think the image is meaningful representing something related to the subject of the blog. It needs a meaningful alt text.
  • wrapping the learning category as a link inside a button gives it two focus when using a screen reader. If it's ment to be a link to lead somewhere I'll just style the link without the button around it. It can also be just an indication without a link and you could use a <p> tag instead so keyboard navigation will go straight to the main link of the card. It will be more logical for me.
  • You mean to put the main H1 as a link so the first element should be the <a> tag instead of the other way.
  • You don't need to wrap the author image in a div. Keep the html simple.
  • The avatar image or usually meaningfull so it needs an appropriate alt text.

  Is it accessible, and what improvements could be made?

  • Currently there are issues of accessibility when using a keyboard navigation there are non necessary focus as mentioned above that can be solved with proper markup.
  • Never use pixels for font sizes and everything related to fonts, sizes, width etc. The current solution doesn't scale correctly in case of a reflow check and doesn't respect the user settings if they change their browser default. Learn how to change your browser default font size and test your solution. If you change your font size to 30px instead of the 16px default, your website should scale correctly. Use relative units and this will all be better :)
  • Center your card on your body. don't use this fixed 130vh on the body. This is a quick fix to center your card.

  body {
      display: flex;
      flex-direction: column;
      justify-content: center;
      align-items: center;
      [+] min-height: 100svh; <-- add this
      [-] padding: 0; <--- remove this
      [-] height: 130vh; <--- remove this
  }
  

  • use relative units as well for your media queries. You want them to scale with the font size setting.

  Does the layout look good on a range of screen sizes?

  • Proper centering will make the layout look better on large screen. It is currently pushed down because of the height fixed to 130vh.

  Is the code well-structured, readable, and reusable?

  • Use a modern css reset at the beginning of your stylesheet. It will help you along the way to handle default browser styling.

  Does the solution differ considerably from the design? it looks good and respects the design. There is one interaction style missing on the card with the box-shadow but overall the solution looks good.

  Try to refactor your solution using these comments. I'm happy to check it again if you want. Don't hesitate to reach out on discord if you have any questions regarding these comments or if something doesn't make any sense for you.

  Happy coding !

  Marked as helpful

  0

  ---

  

  Uzma Khan•160

  @UzmakhPosted 5 months ago

  @gmagnenat => add the title at the top of your head just under the <meta name="viewport".. this is recommanded for performance. Ok, I have done this. => your solution is missing a <main> landmark. Ok, I put the <main> tag in the body => keep the html simple. I don't think you need to wrap that image in an extra div. I disagree with this objection. It is not an extra div it is image container in which image will take its width: 100%. If you have other solution for it , kindly suggest me. => It needs a meaningful alt text. ok, I give it alt = "banner-image"/"Learning basics" => Ok I removed the button tag around anchor tag for Learning => You mean to put the main H1 as a link so the first element should be the <a> tag instead of the other way. Here I changed this code;

  <p class="title"> <a href="#">HTML & CSS foundations</a> </p> => You don't need to wrap the author image in a div. Keep the html simple. ok, I done it. => I have given the alt tags. => Ok I have changed the font-sizes and max-width into rem => Ok I removed the height;130vh

  1

  

  T

  Gwenaël Magnenat•1,290

  @gmagnenatPosted 5 months ago

  Hi @Uzmakh, cool !

  • I see you removed the extra div for image-container. The reason why it may not be needed here because the image will already take the full width of the card. You just need a padding on the card and that's all you don't even need to add the max-width to this image as it fills the whole width of its container.
  • alt text should not mention image as it's already understood by the browser as an image. it needs a meaningful description of its content for when it cannot be seen or displayed. Here is a very good article about this How to write good alt text for screen readers.
  • The H1 for your main title was correct. Probably arguable if there was more then one card on the page. But I was opening a suggestion that maybe wrapping the H1 inside the <a> would be better to make the whole link clickable. (I need to confirm this).
  • For the author image you can simply set it's width in rem instead of % of the parent container.
  • I still see issues in font sizes that are not scaling correctly. The culprit are the 16px font size in your body and the 11px font-size in attribution div.

  Marked as helpful

  0
• 

  Uzma Khan•160

  @UzmakhPosted 5 months ago

  I don't understand the accessibility report.

  0