Putting together HTML and CSS

Community feedback

• 

  Thaís Vieira•120

  @thaisavieiraPosted over 2 years ago

  Congratulations on your first challenge, Ian! Its HTML tags are well organized and the design is very similar. This challenge was also my first one and something that helped me work on responsiveness was https://css-tricks.com/a-complete-guide-to-css-media-queries/ Hope this helps!

  Marked as helpful

  1

  ---

  

  Ian McBee•10

  @imcbeePosted over 2 years ago

  @thaisavieira This was very helpful! I will definitely take a look and read through the documentation and learn more :). Thank you so much!

  1
• 

  Elaine•11,400

  @elaineleungPosted over 2 years ago

  Hello Ian, great job completing your first challenge, and welcome to Frontend Mentor! I think there are many things you did well here, and you managed to get the component looking really close to the original. I honestly think all you need now is just a mobile view, and you're good to go!

  I just got three suggestions for you:

  1. Instead of height: 100% on the body, I'd write min-height: 100vh, and the reason for that is, try changing the browser height to something smaller than the browser's content, and see whether you can still scroll up and down to see the whole component. Using min-height: 100vh on the body would allow you to see all the contents using the scrollbar without it being cut off.

  2. For the image, I also used a background image like you did when I first did this challenge, but later I changed it back to using an img tag. The reason is, with images that relate to the content and aren't actually a background image for decor, you'd want to have it written into the HTML for sure so that the screen reader can pick up on all the info related to the image, such as the alt tag. I know you don't have a media query yet and also no mobile image yet, but this is what you can use when you do write a media query (where the max-width in sizes would be the media query you use in your CSS):

  <img alt="perfume bottle" 
     srcset="images/image-product-desktop.jpg 600w, images/image-product-mobile.jpg 686w"        
     sizes="(min-width: 745px) 600px, 686px" 
     src="images/image-product-desktop.jpg">
  
  // srcset contains the images along with their actual image width in "w"
  // sizes tells the browser which image should be used at the given breakpoint
  

  3. Lastly, I also would add some reset/normalize rules at the top of your CSS just to keep the styles uniform across browsers. I never start a project without one, and currently I'm using Andy Bell's CSS reset rules; you can also look into using normalize.css, which is different from reset rules, but in any case, either one would be good as a starting point.

  Since you only have the desktop view and not the mobile, I assume you started this project with a desktop-first approach. For your next project, you can try using a mobile-first approach, and you'll discover that this allows you to write fewer lines of code to change the layout.

  Anyway, glad you are practicing what you're learning in GA, and hope to see more solutions!

  Marked as helpful

  1

  ---

  

  Ian McBee•10

  @imcbeePosted over 2 years ago

  @elaineleung Thank you so much for the feedback! This was a mountain of wealth and I will definitely refactor this project and implement on the next project! Really appreciate you taking the time for the advice :)

  1
• 

  Lucas 👾•104,340

  @correlucasPosted over 2 years ago

  Hello Ian, congratulations for your first solution and welcome to the Frontend Mentor community!

  Your solution seems great and I'm impressed the level of details you've reached. That's amazing, looking forward to see the next challenges.

  I saw that you did the image import using background-image in the CSS and its not a good price for SEO reasons because this makes harder to the image be found since there's not a proper tag to insert ir and the image comes from the CSS. As an alternative you can manage the product image inserting the tag <picture> to wrap both desktop and mobile images together in the same tag, and render each image depending of the device (phone / computer) by the settings for the width you'll insert in the html If you're not familiar with the picture tag you can look at the documentation to see how to set it:https://www.w3schools.com/tags/tag_picture.asp

  My rating for your challenge ⭐⭐⭐⭐⭐

  Hope this helps, happy coding 👏

  Marked as helpful

  1

  ---

  

  Ian McBee•10

  @imcbeePosted over 2 years ago

  @correlucas I really appreciate your encouragement and the advice! I will definitely implement on the next project that I am working. Thank you so much!

  1