just HTML and CSS

Community feedback

• 

  Shahin NJ•1,190

  @SJ-NosratPosted over 3 years ago

  Hi Dawid, Great solution, it looks identical to the design. You have a good eye for the details.

  Just a few comments, with respect to the accessibility in your HTML, try and use semantic tags. For example: instead of using: <div class="main-wrapper"> do the following <main class="main-wrapper"> that way you provide better experience for screen readers.

  Also good use of the CSS GRID; I should start using it for my layouts. Other than that maybe instead of using pixel, try and use rem and percentages for your widths; allows for more fluid responsiveness.

  Keep up the great work!

  I Would appreciate if you could review my attempt at this solution too; would love your feedback.

  Thanks in advance!

  Marked as helpful

  3

  ---

  

  Dawid Kuczer•110

  @dadko44Posted over 3 years ago

  @shahin1987

  Hello, thank You for your feedback.

  Tomorrow I'll come back an try to encompass your tips into my solution. I'll be more than happy to take a look at your solution too, hope You're having a good day / evening

  1
• 

  T

  Grace•31,270

  @grace-snowPosted over 3 years ago

  Hello

  I think your css approach looks pretty good overall, except the media query is starting too late. You want to set that as soon as there is room for the layout to change.

  More important in my opinion is fixing the html. Numbers like 10k don’t make sense as headings. And capitalisation should be done by css not in the html. I recommend using an unordered list with 3 items for those stats. You can use spans inside for styling.

  Going back to the css

  • never put font size in px, use REM or sometimes em
  • remove any properties you don’t need (word-break? Some of the grid?
  • try using flex for layouts along one axis, grid when you need to lay out on 2 axis
  • only make css selectors complex when you really need to. Keep specificity as low and flat as possible with single class selectors when you can
  • Content is touching screen edges on some screen sizes. Add some padding on the outer wrapper or body to prevent that

  I hope all this helps

  Marked as helpful

  2

  ---

  

  Dawid Kuczer•110

  @dadko44Posted over 3 years ago

  @grace-snow

  Hello, thank You for the feedback. These are all great tips, the only thing I would like to inquire is using unordered list to display the stats.

  The way I saw it was that I have 3 items consisting of a 'title' and 'description' (so to say) being spaced equally horizontally, that's why I thought about using flexbox + 3 divs with h2 for the 'title' and p for the 'description'.

  It just didn't occur to me that I might be looking at a list and I think it's because I'm not really thinking in the terms of HTML semantics good enough yet.

  If I were to use an unordered list to do this should it look like:

  • a horizontal list with 3 items
  • every item has a span to stylize the text and a line break to break the 'title' from the 'description'

  Should it be something like that?

  0
• 

  Mojtaba Mosavi•3,760

  @MojtabaMosaviPosted over 3 years ago

  Good job, the only thing that comes to mind is that you could wrap the stats section in a ul instead of a div because it's more semantic and regarding the usage of max-width/min-width I don't see any unnecessary usage of these.

  Keep coding :=)

  Marked as helpful

  1

  ---

  

  Dawid Kuczer•110

  @dadko44Posted over 3 years ago

  @MojtabaMosavi

  Thank You for your feedback, it means a lot : )

  Right, I got carried away and didn't even think about using ul, I'll keep that in mind

  0