HTML and CSS

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, great work on this one. Layout in desktop seems fine, but the buttons are outside of the container in my screen. I am using 1366x768. Mobile layout seems fine, but 375 is too small for mobile breakpoint. The 375 on the design is not the intended breakpoint, it is just design.

  Some suggestions would be:

  • I suggest not styling the html element. Place those background properties on the body tag. Usually, the html element will only contain:

    html {
    box-sizing: # value stated;
    font-size: 100%;
  }
  

  • Instead of using height: 100vh on the body tag, use min-height: 100vh on it. Using height: 100vh will limit the elements size according to the viewport's height. So even if the element or the website have many element, if it uses height: 100vh it won't logically captures them. min-height: 100vh is much better because it takes 100vh as minimum and it will expand if it needs to.
  • Adding padding to the top and bottom of the body tag so that it prevents the content from touching the sides.
  • In a website, you typically add main element. This makes the structure better and it guides screen reader users where they are when navigating on your website. On this one, the .main-container selector could have been using main element.
  • The alt text on the img should have been left empty like alt="". When you think an image is just being used as a decoration, you should left the alt be empty. If you think the image adds to the context? Use a descriptive alt. Since the image on this one is just a vector decoration, using alt="" is better.
  • The img as well of the music icon should have been using alt="".
  • I would have wrap the annual-plan with a heading tag like h2.
  • Do not position: absolute the footer. Leave it on the flow.

  Other than those, great work. But changing the breakpoint will be really good. Also, if you like, starting some projects using mobile first layout:>

  Marked as helpful

  1

  ---

  

  Jason Moody•300

  @MoodyJWPosted about 3 years ago

  @pikamart thank you, this is great feedback!

  I'm still pretty new to using actual html/css instead of just frameworks and these are exactly the kinds of things I need to learn.

  Can you elaborate on the background-image being in the body element instead of the html element? My understanding is that the html element will always match the height of the browser window and the body element doesn't. I've read this in a few places now, but I generally do see the background image set on the body. Is it just convention or is there something better about using body?

  Again, thank you so much for all of the feedback! Tremendously helpful.

  Edit: also I updated everything and it should be significantly improved, even got the GH pages site to work properly on my master branch. Although the screenshot doesn't seem to reflect what's on the actual live site.

  0

  

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  @MoodyJW Hey, awesome that it helped you!

  On that background-image. Well typically, like what I said, you don't really style the html element and I don't know if html element takes the 100% of the height of the screen? Now, like what I said, put it on the body tag, but at default, body tag does not takes 100% of the screen's height, that is why, min-height: 100vh is typically added to the body tag, so that it will take 100% of the screen's height and it will expand if it needs to. It is different from height: 100vh okay, always go with min-height: 100vh and not height: 100vh since the latter one will limit an element to only have 100% of the viewport's height.

  Also, there is this "generate new screenshot" which you can use on your solution's page. This will take the new updated version of your website as the screenshot in here.

  Great work once again!!

  Marked as helpful

  1