Notification Page

Community feedback

• 

  Aman Singh Bhogal•1,010

  @asbhogalPosted over 1 year ago

  Hi there,

  Good work! I can see you've made a solid attempt at matching to the mockups. I've noticed several things however which are worth considering for refactoring this:

  • You can use the font-clamp() property to dynamically scale down the font-size from a value set at a high viewport width down to the the lowest set viewport width in rem (note that this assumes a base font-size of 16px which is suitable generally, but it's worth noting in larger applications this would require further adjustments.) Here's a link to generate the function Link
  • Your markup is also semantically incorrect. The <header> should always be at the top, outside any other element (except the body) and your main content should be wrapped in the <main> element to mark where the most interactive content is (important for accessibility.) You could remove the two <div> elements and style the first two I mentioned.
  • It's great to see that you've locally hosted your font, however this should be in .woff2 format as this is designed for the web. Here's a link explaining this in further detail Link
  • Your <img> elements are missing alt attributes, which are important for screen readers and other assistive technologies to be able to read them to establish their purpose and context
  • Avoid using var when declaring variables - I noticed this in your var classname = span.getAttribute('class');. This should be const seeing as it isn't being reassigned within the same scope

  Hope this helps!

  Marked as helpful

  1

  ---

  

  jr19981010•90

  @jr19981010Posted over 1 year ago

  @asbhogal Hello Aman! Nice review. But please enlighten me. if I remove the height: 100vh, the container would not be centered vertically.

  1

  

  Aman Singh Bhogal•1,010

  @asbhogalPosted over 1 year ago

  @jr19981010 hi there, I had a look again at your code and made an error, the min-height: 100vh looks fine in this case. (The responsive viewer I used in dev tools was cutting off the content, not the app itself.) My apologies, I've edited my original comments to reflect this. Thanks for mentioning.

  1

  

  jr19981010•90

  @jr19981010Posted over 1 year ago

  height: 100vh this is the original property. I just changed it to min-height: 100vh. Thank you also for the information about clamp().I learned a lot from your feedback.

  0