Review suggestions #14
Conversation
macfrei
left a comment
macfrei
left a comment
There was a problem hiding this comment.
Hope this makes sense for you! If you have any questions, please don't hesitate to ask. I hope I didn't break your side 😄. When adding my suggestions, maybe do that in an own branch and don't merge mine.
| @@ -0,0 +1,19 @@ | |||
| .aside { | |||
There was a problem hiding this comment.
As this is it's own component, I would add a dedicated css file for it.
| @@ -0,0 +1,17 @@ | |||
| .cta { | |||
| background-color: var(--pale-cyan); | ||
| } | ||
|
|
||
| .header__top-container { |
There was a problem hiding this comment.
I think here it is an element of your header block (BEM), with the other one it is a new block you are starting.
| .header__bottom-container { | ||
| display: grid; | ||
| grid-template-columns: 50% 50%; | ||
| .bottom-container { |
| @@ -0,0 +1,6 @@ | |||
| .main { | |||
There was a problem hiding this comment.
This is the tile-wrapper style. I think leaving out the div and styling the main directly is a better solution.
| @@ -0,0 +1,100 @@ | |||
| @media only screen and (max-width: 1000px) { | |||
There was a problem hiding this comment.
I would suggest adding these in its own css file as well.
| padding-top: 2vh; | ||
| } | ||
|
|
||
| .tile__p { |
There was a problem hiding this comment.
Usually when using BEM or adding CSS classes it is not common to cite the HTML element. Would be better to name it according to the content.
| >[email protected]</a | ||
| > | ||
| </div> | ||
| <div class="contact__item contact__learn-more" aria-label="Learn more:"> |
There was a problem hiding this comment.
Usually aria-labels are used for buttons, links or images or something like that.
1 participant
Nice job! I changed a few things that I thought would be a bit unnecessary. I will explain in comments 🙂