Hi Michelle,
Really nice work on this. Text is too small for me to read without zooming but overall. it's one of the best of these I've seen. Particularly good use of BEM naming on your classes.
Main points for improvement are in html:
- I don't think you need alt text to identify a background pattern. Leave alt attributes intentionally blank if they don't offer additional meaning for assistive tech users
- those stats all need to be in one html tag really. You could use spans with classes to change the sizes inside a paragraph tag. At the moment, titles like '803k' don't really make sense.
- as mentioned above, if you want to shrink html font size like that, that's OK. But you have to remember to make sure no text ever goes below about 12px, and that should only be for smalltext. Shrinking html size means you have to set fonts on everything to be bigger so it's not something I advise unless youre really sure.
Can i ask, did you write the css as scss first by any chance? Just wondered...
Anyway, well done again and happy learning ☺
@michelleracho
Posted
Hi Grace,
Thanks so much for the feedback! I'll fix the html and definitely keep those points in mind and yes, I wrote the css as scss 🙂
update: I fixed the stats and the font size but I decided to leave the alt on the background because I remember netlify not liking me leaving those out 😅
@michelleracho you always want to have an alt attribute, but leave it empty to tell assistive technology they can skip it, like alt=""
Why havent you included the scss in the project? Or did I miss it in a folder somewhere 😂 - It's good to be able to review how scss is being written too sometimes.
Well done again on this
@michelleracho
Posted
@grace-snow oooh thanks for that tip! I didnt know that was ok 😅
the scss files are on my github in a "sass" folder not sure if scss would be a better name for it tho 😂
https://github.com/michelleracho/profile-card-component/tree/main/sass
@michelleracho yep, thought I probably just missed it! 😅
Very nice sass. Only thing I'd change is nesting of classes like .followers inside another class. That's increasing specificity, so although not bad exactly, can be a bad habit to fall into that can bite you later in bigger projects.
@michelleracho
Posted
@grace-snow oh that makes sense! so dont nest a class if it's not necessarily an "element" under the "block"
@michelleracho I'd go further than that. I just don't nest at all in the css unless I need to increase specificity. But because I don't nest, that's really rare that would be needed 😉
The only things I nest under a class in scss are... Pseudo elements like before, after last child etc, media queries, and sometimes direct descendent or sibling selectors.
E.g. I might do something like
.reviews-grid {
display: flex;
// define grid styles and spacing like margins etc or global style for this component like font family if needed
> * {
flex: 1 1 300px;
// positioning styles for the grid items only include what relates to the grid above
}
}
.reviews-grid__item {
// any other styles for the grid items like color, their display properties etc
&:last-of-type {
margin-right: 0;
}
}
No matter what, it's very rare css I write will use 2+ selectors to set a style. The only exception is when trying to overwrite some third party css (eg. bootstrap is horrendously over specific)
@michelleracho
Posted
@grace-snow oh I see what you're saying now! The way I learned sass was to basically nest the B-E-Ms but this is a fresh approach to nesting 😀
