empty pull request #2659
empty pull request #2659
Conversation
There was a problem hiding this comment.
Well done. 🥇
I found some BEM mistakes to fix in the comments. The code is really well-structured and easy to read. Also, consider using higher quality images. It's good practice to have pictures at least 2x larger than the actual width and height that you display on the web.
src/images/photos/3.jpg
Outdated
There was a problem hiding this comment.
You can take higher resolution images. In Figma, there is a link for a higher resolution image used on the right sidebar. Additionally, it would be beneficial to minify those images as it can reduce their size by around 70%.
src/styles/utils/_variables.scss
Outdated
| @@ -0,0 +1,9 @@ | |||
| $color-accent: #d12d35; | |||
There was a problem hiding this comment.
I am a bigger fan of naming things by their color. Without context, it is hard to tell what is what, and it's easy to forget. For example, calling it 'color-red-dark' instead of just 'color-main' makes it easier to identify and remember.
| <a | ||
| href="tel:+12125357710" | ||
| class="navigation__icon navigation__icon--phone" | ||
| ></a> |
There was a problem hiding this comment.
It is good if links that have only icons include a tooltip and aria-label for better accessibility.
src/index.html
Outdated
| > | ||
| <div> | ||
| <p class="title__description">Plan your visit</p> | ||
| <h2 class="title--secondary information__title">Museum hours</h2> |
There was a problem hiding this comment.
if you want to use modifier then you should also use original block or element that the modifier is applied on. As i saw in styles there is some repeated code for title and title--secondary
src/index.html
Outdated
|
|
||
| <div> | ||
| <article> | ||
| <h3 class="title--tertiary information__title"> |
| id="information" | ||
| > | ||
| <div> | ||
| <p class="title__description">Plan your visit</p> |
There was a problem hiding this comment.
It is element but I can't see the parent of this element.
src/index.html
Outdated
| alt="2" | ||
| class="exhibition__photo" | ||
| /> | ||
|
|
||
| <img | ||
| src="./images/photos/3.jpg" | ||
| alt="3" | ||
| class="exhibition__photo" | ||
| /> | ||
|
|
||
| <img | ||
| src="./images/photos/4.jpg" | ||
| alt="4" |
DEMO LINK