solution (https://232567.github.io/layout_creativeBakery/).#767
Conversation
DanielCamposDevX
left a comment
There was a problem hiding this comment.
What we bake section is on the far left instead of centralized, proportions are a bit odd on bigger screens
|
Hello! Thank you very much for your detailed review and feedback. I have addressed all the points you mentioned: Added the favicon. I also reviewed the project again to make sure everything matches the checklist as closely as possible. Thank you again for your time and feedback. I look forward to your new review. DEMO LINK |
raulriato
left a comment
There was a problem hiding this comment.
You are almost there.
I'm requesting just a few changes in order to use the Sass maximum powers and fix the only great problem in your code.
The recommendations about the mixins and the nesting should be fixed in every place you have them.
Also, it seems you coded your page to be desktop first, it means your standard styles work in the desktop viewport. In that case, you will not need the mixin on-desktop, because it is already your standard styles. You will use on-tablet (for max-width: 1199px;) and on-mobile (for max-width: 767px;)
| /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>Creative Bakery</title> | ||
| <link rel="icon" href="./src/images/favicon.png"> |
There was a problem hiding this comment.
You are pointing the href to a favicon.png, but there is no such file in the images folder. Use cookie.png instead.
|
|
||
| .products__grid { | ||
| display: grid; | ||
| grid-template-columns: repeat(2, 280px); |
There was a problem hiding this comment.
This is causing the cells to be smaller than it should be and not centered. You should have a grid with more columns to rearrange each product. You could also, in this case, just use flex
| color: #7f8096; | ||
| } | ||
|
|
||
| @media (max-width: 1279px) { |
There was a problem hiding this comment.
You should not have your @media hardcoded in each file. The mixins are meant for this. You should have, for example:
@mixin on-desktop {
@media (min-width: 1279) {
@content;
}
}And here you would use just:
@include on-desktop {
/*the styles that will change on desktop*/
}| .about { | ||
| position: relative; | ||
|
|
||
| padding: 80px 120px 120px; | ||
|
|
||
| background-image: url('../images/crumbs.png'); | ||
| background-repeat: no-repeat; | ||
| background-position: center 70px; | ||
| background-size: 430px; | ||
| } | ||
|
|
||
| .about__content { | ||
| display: grid; | ||
| grid-template-columns: 360px 420px; | ||
| gap: 80px; | ||
| align-items: start; | ||
| justify-content: space-between; | ||
|
|
||
| margin-bottom: 100px; | ||
| } | ||
|
|
||
| .about__title { | ||
| margin: 0; | ||
|
|
||
| font-size: 56px; | ||
| font-weight: 300; | ||
| line-height: 0.9; | ||
| text-transform: uppercase; | ||
| letter-spacing: -1px; | ||
| } | ||
|
|
||
| .about__text { | ||
| margin: 0; | ||
| font-size: 14px; | ||
| line-height: 150%; | ||
| } |
There was a problem hiding this comment.
You are using scss now. One of the greatest features of scss is the possibility to nest different styles. You should use it:
.about {
/*the styles for the class "about" go here*/
&__content {
/*the styles for class "about__content" go here*/
}
&__text {
/*the styles for class "about__text" go here*/
}
/*...*/
}
DEMO LINK.