@steventoben
Posted
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!!
@astridv
Posted
@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 withdisplay: block
and some other property?
Once again thank you for your valuable help! 😄