The Met #2046
The Met #2046
Conversation
There was a problem hiding this comment.
Good job overal
However, you somewhat misuse BEM and need to add some fixes to your project structure
The intent of BLOCK in BEM that blocks are reusable. If you have the block with 2-3 lines of styles - that should not be a block. If you have two blocks that are only used together - please combine them into one
Also please use nesting - it is the wonderful scss feature, that makes the code way more readable
You need to reorganise your project structure and reduce the number of blocks (for instance you don't need body_nav-menu and nav-menu separately)
And one minor visual issue:
Probably you should add transform on hover on each picture separately, not on the whole section
| .container { | ||
| max-width: 1020px; | ||
| margin: 0 auto; | ||
| padding: 0 20px; | ||
| } | ||
|
|
||
| .section-wrapper { | ||
| margin: 100px auto; | ||
|
|
||
| &:last-child { | ||
| margin-bottom: 0; | ||
| padding-bottom: 100px; | ||
| } | ||
| } |
There was a problem hiding this comment.
It is not the best idea to use these classes like this
They should be either real BEM blocks (separate files, no margins (!), etc) or elements (then you need to put them inside their parent blocks files)
There was a problem hiding this comment.
There are multiple issues with all this file - you have multiple classes/blocks here, that are poorly organised and thus are nearly unsupportable
Please make an effort to clean up this file. Here are the possible options, what to do with the classes:
- Some should go to other blocks as the block elements
- Some should be converted to independent blocks
- You may also convert some classes to mixins, and reuse them where you need
| } | ||
| } | ||
|
|
||
| .main-btn { |
There was a problem hiding this comment.
This can be converted to a block - and go to a separate file
| html { | ||
| scroll-behavior: smooth; | ||
| } | ||
|
|
||
| body { | ||
| font-family: Cinzel, sans-serif; | ||
| margin: 0; | ||
| } |
There was a problem hiding this comment.
This and all styles on the tags bellow are totally ok to keep here
| .about-us { | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 32px; | ||
| } | ||
|
|
||
| .about-us__subtitle { | ||
| font-size: 12px; | ||
| line-height: 16.8px; | ||
| text-align: left; | ||
| } |
There was a problem hiding this comment.
Here and in other similar places - please use nesting
| .about-us { | |
| display: flex; | |
| flex-direction: column; | |
| gap: 32px; | |
| } | |
| .about-us__subtitle { | |
| font-size: 12px; | |
| line-height: 16.8px; | |
| text-align: left; | |
| } | |
| .about-us { | |
| display: flex; | |
| flex-direction: column; | |
| gap: 32px; | |
| &__subtitle { | |
| font-size: 12px; | |
| line-height: 16.8px; | |
| text-align: left; | |
| } | |
| ... |
| .collection { | ||
| display: flex; | ||
| flex-direction: column; | ||
| align-items: center; | ||
| gap: 48px; | ||
| } |
There was a problem hiding this comment.
collection is a separate block and should go to a separate file
| .nav { | ||
| display: flex; | ||
| gap: 24px; | ||
| } |
There was a problem hiding this comment.
if these are all styles - why you need .nav as a separate block at all?
Just include these styling to header__nav
| .nav-menu { | ||
| width: 100%; | ||
| height: 100vh; | ||
| background-color: $nav-menu-bg-color; | ||
|
|
||
| transition: 0.3s; | ||
| opacity: 0; | ||
| z-index: 2; | ||
| pointer-events: none; | ||
| transform: translateX(100%); | ||
|
|
||
| &:target { | ||
| opacity: 1; | ||
| transform: translateX(0); | ||
| pointer-events: all; | ||
| } | ||
| } | ||
|
|
||
| .body__nav-menu { | ||
| position: fixed; | ||
| top: 0; | ||
| left: 0; | ||
| right: 0; | ||
| } |
There was a problem hiding this comment.
why you need separate nav-menu and body__nav-menu?
[DEMO LINK] [https://sansamiste.github.io/layout_landing-page/]