four-card-feature-section

Community feedback

• 

  T

  Grace•31,270

  @grace-snowPosted 9 months ago

  Some more feedback

  • Don't use <br/> to force line breaks. Some screen readers will read those out as "break!" Instead, this has different styling on each half. Use a strong or span set to display block for the second line
  • Learn how and when to write alt text. There is a good post about this in the resources channel on discord
  • you haven't used a modern reset as recommended
  • you don't need to define row heights. That could cause problems.
  • Do not add min-width on the grid. You don't need it and it will cause overflow bugs for some users.

  Marked as helpful

  1

  ---

  

  Diwakhar•460

  @diwakharpandyanPosted 9 months ago

  Thanks @grace-snow for the br suggestion... I didnt use pseudo classes so i didnt use modern reset... And I havent defined any row heights in this re-uploaded solution grace... I added min-width to prevent layout from getting shrunken below small screen size 320px....

  0

  

  T

  Grace•31,270

  @grace-snowPosted 9 months ago

  @diwakharpandyan the lack of reset is causing overflow.

  The min width is causing overflow.

  1

  

  Diwakhar•460

  @diwakharpandyanPosted 9 months ago

  Ok @grace-snow I will remove min-width... Can u send me the reset code grace? I saw this on andy bell site...*, *::before, *::after { box-sizing: border-box; } For universal selector i have already applied but i haven't used before after pseudo classes so pls u send the reset code that u r referring...

  0

  

  T

  Grace•31,270

  @grace-snowPosted 9 months ago

  @diwakharpandyan use the full reset not just the first few lines. There are very good articles about why it includes everything

  Marked as helpful

  1

  

  T

  Grace•31,270

  @grace-snowPosted 9 months ago

  @diwakharpandyan just keep the full reset at the start of every project. This is a much shorter reset than most. Compare to normalize.css that one is HUGE

  1

  

  Diwakhar•460

  @diwakharpandyanPosted 9 months ago

  Hey @grace-snow i just saw the whole reset... So u want me to copy the whole reset at the start of every project even though I haven't used some of the elements in my project like a, figure, blockquote, input, button, before, after, label etc..,right? It has about 10 code blocks...

  0

  

  T

  Grace•31,270

  @grace-snowPosted 9 months ago

  @diwakharpandyan yes. Get into that habit. You wouldn't mess around with a reset. You just use it at the start of every project.

  I can't believe this has needed so many messages. I feel done now.

  0
• 

  T

  Grace•31,270

  @grace-snowPosted 9 months ago

  Try and get this closer to the design if you can. Eg

  • max width on the paragraph
  • larger font sizes
  • increased line height

  Small things like this can make a big difference.

  A few other things

  • the main heading is one heading, not two. It needs to be read all as one.
  • once that's fixed the card headings will need to become h2s
  • page specific content like headings don't belong in a header element. That's a landmark with a banner role, meaning it is for important repeating content across every page of a site, like logo and nav, and it sits outside of main. All of the content in this design belongs inside main, not in a header.
  • the icons are decorative so should have empty alt.
  • never limit the height of elements that contain text, including the body. It's fine to use min-height but not height as it can cause difficult bugs.
  • Get into the habit of using 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.
  • Font size must never be in px
  • Don't use keywords for font size either. They can differ between browsers and means a solution will never consistently match a design.
  • I'm away from computer so can't inspect what's happening on larger screens, but I am seeing css I definitely wouldn't expect. I expect to see a single column grid by default for the mobile styles, then a grid where the card area has 3 columns and 4 rows. I wouldnt expect to see any % heights anywhere. The grid template handles all sizing of the children usually. I think you really need to rewrite some of this grid as it's showing a misunderstanding of how to use it to its full potential. This is the main skill this challenge is designed to stretch. Give it the time.
  • Keep CSS as low specificity as possible. Try to avoid complex css selectors like nested or direct children selectors. Use single class selectors instead whenever possible (put classes directly on what you want to style)
  • Media queries should always be defined in rem or em not px. The mobile styles should be the default. Then override specific properties that need to change in media queries for larger screens.

  Marked as helpful

  1

  ---

  

  Diwakhar•460

  @diwakharpandyanPosted 9 months ago

  U r right @grace-snow ... I did and upload in hurry... I should have given more time to it... U have given a comprehensive review ... I will re-upload the solution with all the corrections u said including the mobile -first approach... So pls come back once again to validate my code after i re-upload... Thanks...

  0

  

  Diwakhar•460

  @diwakharpandyanPosted 9 months ago

  Hey @grace-snow. I have reuploaded my solution. Need ur review. Thanks.

  0

  

  T

  Grace•31,270

  @grace-snowPosted 9 months ago

  @diwakharpandyan another note - stop updating solutions via file upload. You are losing all the benefits of git version control. Learn to push and pull and commit properly as soon as possible

  Marked as helpful

  1

  

  Diwakhar•460

  @diwakharpandyanPosted 9 months ago

  Ok @grace-snow ...im yet to learn git version control.

  0