Skip to content
OverviewSolutions
3
Comments
5
Jessica Strachan
@JessicaStrachan

All comments

  • Jessica Strachan•160
    @JessicaStrachanPosted almost 4 years ago

    Nice work :)

    One pointer I have is to consider using a max-width on the cardWrapper. The use of just percentages runs the risk of the card looking too wide on larger monitors. Then you could potentially look at making it width: 100% on smaller devices, relying on the margin or main element to prevent the card from touching the edge of the screen.

    1
  • Jessica Strachan•160
    @JessicaStrachanPosted almost 4 years ago

    Nice use of CSS variables :)

    Some feedback I have would be refraining from setting class names that you're not going to use to style with, eg. followers, likes and photos. I can see that you have styled them using .stats div so my next point is that it's best practice to generally avoid styling directly onto HTML elements where you can. Personally I would go for something like stats-item or if you're using the BEM methodology (which I love!), stats__item.

    1
  • Jessica Strachan•160
    @JessicaStrachanPosted almost 4 years ago

    I don't see anything wrong with your use of BEM naming Theodorus, I think you've used it well :)

    If you wanted to extend it further you could use it on the classes main-title and text-small, for example: card__title and card__text. I think you are showing use of class names being reused with your typography classes and card__social. However I would personally lose the card__social class as you're not applying any styling so it wouldn't make a difference if they were just plain divs.

    2
  • Jessica Strachan•160
    @JessicaStrachanPosted almost 4 years ago

    Nice work Jaime :)

    Some feedback on the body background images: On line 13 of style.css, you're actually overwriting the line before, they're essentially the same thing, and you're seeing two of those background circles because the background images gets repeated by default. You can prevent this with background-repeat: no-repeat.

    In order for these circles to be treated as separate elements you could either put them in separate divs with unique classes and then you will be able to rotate those individual elements with transform: rotate(90deg) for example. However I think these assets are supplied the same as the design so I don't believe you should need any rotating in this case.

    I would also suggest using classes instead of ids. Ids have high specificity than classes, and whilst it's not incorrect, you could cause yourself future problems in more complex styling with specificity wars or finding the need to use !important to override styles which isn't recommended.

    1
  • Jessica Strachan•160
    @JessicaStrachanPosted almost 4 years ago

    Naming can be so tricky sometimes! Good effort on the BEM so far, however I did notice one thing:

    Lines 27 and 32 card__content__top and card__content__bottom are quite different and going by your css they don't have many styles in common for them to be considered the same element of __content. I would personally call them card__main and card__footer. Or if it was a section at the top card__header.

    In the instance that you did have styles that were mostly the same, just a different positioning you would use a BEM class like so: class="card__content card__content--top" and class="card__content card__content--bottom" and this is where using SASS really helps with using BEM classes because of the nesting:

    .card
    *card styles here*

    &__content
        *card__content styles here*

        &--top
            *Unique styles for the top*

        &--bottom
            *Unique styles for the bottom*
    2