Responsive grid testimonials using flexbox, CSS grid and BEM

Community feedback

• 

  Steven Toben•750

  @steventobenPosted almost 4 years ago

  So first off you're getting complaints about incorrect html semantics because you are using the section and article tags incorrectly. A section element is requires there to be a heading element. Think of a section as a section in a blog post, usually a blog will have subsections that talk about specific topics, so a section would be used like that, where the heading is the title of the section and then it contains other content. It looks like you're trying to use a section as a grouping tool. This is generally what a div or a span element is for. Divs would be the proper tag to use in most of your cases here.

  You used an article element to create your cards, this could just be represented in a div tag instead. It looks like you used a section element for creating the user info component at the top of the cards. The user info component could also use a div tag as the container, and inside of that container you have the avatar component, and the user info text component. You could keep the avatar as you have it, just an img element sized to the picture given. Then the user info component shouldn't be a section, but instead a span. You want to use a span since you want the text to be positioned inline (div components display as block, so multiple divs will stack on top of each other, spans will display in the same row by default). In that span you'll have two elements for the two texts that you need to display. You could use a low level h element for the name, but a p will work too. The other component will be a p element. Then style them with appropriate styles, the name should have a heavy weight, darker color, and possibly a slightly higher font-size. The text below should be a lighter color with a thinner weight, and should probably have a fairly decent amount of letter-spacing.

  Also the profile card looks pretty off center on my browser, I'd personally reduce the margin you have between the two texts. You don't need to use flexbox on this component, but since you are you could trying to align-items by center or baseline to get a more centered looking user card.

  The cards themselves look quite nice, one thing I would say is that for setting the body text color on your cards to a lighter color, I would use the rgba(x,x,x,x) function to create the color value. Changing opacity affects the entire element's opacity, where as if you set color=rgba(200,200,200,0.6) or something like that you could set just the text color to a greyish color with 60% opacity.

  Also the grid you put on the body element is completely unnecessary, Setting the display to block would have the same effect with much less complication.

  I don't think setting your base font-size to 13px is a very good idea. If you want to declare a base font-size you should do it in the html selector and you should set it to 100%, so it uses the browser's value and is scaled for the user who may need a bigger font because they can't see well. In general you shouldn't ever set a font-size in pixels, you should always set the font size for an element using rem units. In general it's best to avoid pixels and any absolute unit. You did a really good job on not relying on absolute positioning as well as fixing your width and heights. Also it might look a bit better if you set values for line-height, generally 1 - 1.2 is nice for headings, and 1.5 is good for paragraphs. You don't need units when setting line-height, it's simply a multiplier relative to the font-size.

  Also just a note, be careful with em units and make sure that you know how they're being used when using them. They can be dangerous and compound sizes real quick.

  Overall I think this looks pretty great and the responsiveness looks real nice. For your BEM question, I think they look about right, personally I don't use that method of naming so I can't give much advice, but the naming seems to make sense. Also I'm not completely sure I understand what you're asking with margins, but I will say that it's super common practice to change the box-sizing to border box, and setting margin and padding to 0 in the * selector class which sets those common styles to everything in your page. The last thing I'll say is that I think you actually could get away with no media queries on this. It's possible to make a responsive grid where if something like a column width falls below or above a certain value, it will adjust the grid. Just mentioning that because I use that quite often and it's a really useful technique. But yeah overall I'd get rid of the article and section elements, and use div or span elements as containers. I also think you could adjust row heights based on the card heights which would reduce whitespace at the bottom of some cards. Good job tho it looks pretty great in general!!

  4

  ---

  

  Astrid•100

  @astridvPosted almost 4 years ago

  @steventoben wow so, first of all, I'm absolutely blown away that you took the time to write such a detailed and well-explained response!! 👏 Thank you very much for all your feedback, I appreciate it a lot 🤩 I went through all your feedback and have implemented most of what you suggested, and I think it looks a lot better now 😊 I also learned a lot from reading through your comments and will definitely keep it in mind for future challenges.

  Just a quick follow up question if you have the time:

  • The grid on the body component is there (in combination with place-items: center) to vertically and horizontally centre the wrapper class (both on mobile and desktop). Could I achieve the same effect with display: block and some other property?

  Once again thank you for your valuable help! 😄

  0
• 

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted almost 4 years ago

  Greetings, Astrid! 👋

  Of course, it's nice to see you complete yet another challenge! 😀 Good job on this one! 🙌

  Looks like you already have some great feedback, so my 2 cents would just be to add a max-width to the grid to prevent the testimonial cards from becoming too stretched on extra-large screens (just a small design thing). 😄

  As always, keep coding (and happy coding, too)! 😁

  1

  ---

  

  Astrid•100

  @astridvPosted almost 4 years ago

  @ApplePieGiraffe thanks once again for your useful feedback! 😍 I've added the max-width as you suggested 😄 I checked on mobile, but completely forgot about extra-large screens. Definitely a good tip that I'll keep it in mind for future challenges ✨

  0
• 

  Szymon Rojek•4,540

  @SzymonRojekPosted almost 4 years ago

  @steventoben and @astridv

  Well done explanation @steventoben. Yes it is not a good practice to put a section inside of an article. To be honest for me it is always hard to implement an article and section because it is very easy to overuse these semantic tags.

  Article elements are good for content that could stand alone by itself. For example: blog posts, news articles, forum posts. What's about section: this element we can use for larger groupings of related content. For example, the feedback section on the solution page.

  In my opinion it is not a mistake when each of testimonial will be an article and inside of it will add additional header, blockquote, heading, paragraphs, img. If we see this project as a single page component then we can add the main tag, section, h1 with the class sr-only set to hidden for the Screen Readers, then name of the person can become the h2, rest of the texts will become paragraphs. All depends on the context, content etc. Also, bold text doesn't have to indicate on heading. What do you think?

  From the other hand possibly we can use a section here just for groupings of related content (wrapper). Also, inside of a section we can add 5 divs (each testimonial). I agree with you that divs are perfect for generic groupings of content.

  @steventoben I like your comments and the way how you did explain all details.

  Ps. Don't forget to upvote any comments on here that you find helpful.

  Greetings :D

  1

  ---

  

  Astrid•100

  @astridvPosted almost 4 years ago

  @SzymonRojek thanks a lot for sharing your thoughts and your explanation about article and section elements 😄 I definitely see what you mean about how it is easy to overuse them 😂 I also realise now that I misunderstood section, I thought it could be used for any "section" of content, whether large or big, so thanks for clearing that up for me!

  My idea, initially, was that the solution represented the whole website, and therefore each testimonial card could be seen as a standalone article (sort of what you described in your third paragraph) with its own content inside. But in reality, it would probably be just a small part of a larger site with more content. I, therefore, followed steventoben's and your second suggestion to use a wrapper with five divs representing each testimonial 😊

  Thanks again, and have a nice day 😄

  0