First Project

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted almost 3 years ago

  Hey, great work on this one and congrats to your first challenge here in FEM^^. Like what Olesia Kissa said, the desktop layout is missing both the background-color and background-image, checking that one out would great.

  Some other suggestions besides Olesia Kissa's awesome feedback:

  • It would be great to have a base styling of this:

  html {
    box-sizing: border-box;
    font-size: 100%;
  }
  
  *,
  *::before,
  *::after {
    box-sizing: inherit
  }
  

  This way, handling an element specially its size will be easier because of the box-sizing

  • Don't use position: absolute a very large element. If you inspect your layout, hover on either html or body tag, you will notice that it has no height since its element is being absolute. Since you are just using this to center the layout, it would be much better to do it this way. But first, remove these stylings on the .container:

  position
  top
  left
  transform
  

  Then on the body tag use these stylings:

  align-items: center;
  display: flex;
  justify-content: center;
  min-height: 100vh; 
  

  • The vector/illustration on this is just decorative. Decorative images should be hidden for screen-reader at all times by using alt="" and aria-hidden="true" to the img tag or only aria-hidden="true" if you are using svg instead of img tag.
  • Only use descriptive alt on images that are meaningful and adds content to the site otherwise hide the image for screen-reader users.
  • article is not needed on this one. You typically use article when the content can be distributed in different sections or pages since the content is independent on its own. Change it to using p tag.
  • Also, you don't need to use br on those to make each lines wrap in another row. You can just increase the padding on it so that it will limit the size of the text, making text go in another row if needed. The left and right side of padding.
  • The music-icon as well is only decorative so hide it using the method above.
  • Avoid using id to target and style an element since it is a bad practice due to css specificity. Instead, just use class to target element.
  • The annual plan and the pricing text below it are two separate text. For that one, you can use heading tag like an h2 on the annual plan since it gives information about the section's content and use p tag on the pricing.
  • Lastly, just adding the background-color and background-image for the desktop layout.

  Aside from those, great job again on this one.

  0

  ---

  

  Simon•50

  @simon-baumhauerPosted almost 3 years ago

  @pikapikamart Thank you so much for this thorough answer, I try now to implement all your corrections. I'm so surprised by this effort to help me. Thank you!

  1
• 

  Olesia Kissa•110

  @olesiakissaPosted almost 3 years ago

  Greetings, @simon-baumhauer! The markup looks good, however the styling needs some fixes:

  • I would recommend you to check your body background property on both layouts because the color disappears once the viewport is > 375px and also the background url seems to look for the svg pattern file that's not in your project folder. You should update your repository with the corresponding images and I think it should work fine.
  • Your annual plan container background color isn't the one specified in the style guide, make sure to check out the codes for the color.

  Good luck on your next proejcts :)

  P.S. check out the title tag in your html file, if you leave it as empty it may cause issues during validations.

  0

  ---

  

  Simon•50

  @simon-baumhauerPosted almost 3 years ago

  @olesiakissa Thank you very much!

  0