CSS

Community feedback

• 

  Richard Ogujawa 👨‍💻•170

  @RichardOgujawaPosted almost 2 years ago

  @Teles23 No worries man, more than happy to help! Best of luck with coding going forward, I'm sure with an attitude like the one you have right now you'll do great things:)

  0
• 

  Richard Ogujawa 👨‍💻•170

  @RichardOgujawaPosted almost 2 years ago

  Hi there,

  Hope you're keeping well:) I really like your coding solution, especially the fact that you indented your code. It makes it easy to read. I appreciate that you used the different images instead of just scaling it down for the mobile version, didn't see many other people doing that, unless I wasn't looking well enough.

  However, I would make some changes to the code. Then again these are just my opinion so feel free to take this on board or take it with a grain of salt, I wish you all the best either way.

  In the HTML:

  • I wouldn't recommend using a line break (<br>) after your paragraph. It's a block element so it will take up the full width of its parent container anywhere and forces any content beside it in the HTML onto a new line. If you were using it to create a space between the paragraph text and the heading, I also wouldn't recommend it because you could easily achieve that with a margin-bottom in CSS which gives you more control of the height of the space between the two elements. Actually you could size the height of the br by selecting it in the CSS, but that just results in more lines of code than you need honestly.
  • I'd recommend looking into BEM Naming Conventions for naming classes. It makes it easier for you to write your class names without having to think too much about it, and it also makes it easier other developers to understand your classes, because the naming convention is quite a logical one. There are two articles I'd recommend reading on it, and once you read them you're pretty much set. The first one talks about how to write BEM (https://codeburst.io/understanding-css-bem-naming-convention-a8cca116d252 ) and the second one talks about common problems you may run into when writing BEM and how to overcome them (https://medium.com/fed-or-dead/battling-bem-5-common-problems-and-how-to-avoid-them-5bbd23dee319 )
  • A product image is part of the content of the site, it shows the audience what the product their going to be buying looks like, in otherwords it's not there for decoration, so I wouldn't recommend using the background-image: url() method to display the image, because you can't include any alt text when you use that method.
  • If you want to change your images out at smaller screen sizes, like I saw you did in your CSS, I'd recommend doing this in the HTML using a picture tag. https://blog.bitsrc.io/why-you-should-use-picture-tag-instead-of-img-tag-b9841e86bf8b
  • Also a product card is a piece of content that can be easily understood without any other content around it so instead of just using a div for it, I'd recommend using an article tag. More on that here: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/article
  • The prices aren't really a list of anything, so I wouldn't recommend using an unordered list for it.
  • If you want a space between the Add to Card text and the icon I wouldn't recommend using   because once again you can't control the size of that. Instead I'd recommend just leaving that to when you get to the CSS. You could do something like wrap the Add to Cart in a span, and then simply do a display:flex on the button, and then add a gap between the two flex-items inside (the icon and the button text)
  • Icons should have alt text unless the text beside them is pretty much the same as what you would put in the alt text, in that case, it becomes redundant to include alt text because the screen reader would simply be reading out the same text twice.

  In the CSS:

  • An easier way to center content in a container would be to use {display: grid; place-content: center;}
  • I'd recommend using em and rem units. Em units for things like padding, margins, border, etc. and rem units for fonts. They are both linked to the font sizes you use so with this method you can easily adjust the size of a lot of things on your page simply by adjusting the font-size in the html selector. Also because em is linked to the font-size, you don't have to scale up buttons if you increase the font-size, it'll do so automatically.

  Hope this helps:) And if you want me to explain anything a bit better just let me know, I was trying to keep this not too long, so I had to rush through some concepts.

  0

  ---

  

  Thiago Teles•80

  @Teles23Posted almost 2 years ago

  @RichardOgujawa Primeiramente muito obrigado pelo feedback! Você trouxe os pontos que realmente eu tive dificuldade principalmente em como utilizar duas imagens diferentes para telas diferentes, por isso optei por usar como Background. Estarei corrigindo esses pontos e melhorando cada vez mais, muito obrigado!!

  0