Conversation
LucyMac
left a comment
There was a problem hiding this comment.
Great work Raj. Good use of Scss, and the Bem notation! Your code is clear and easy to follow.
The end result looks good. It's probably a little narrow on a small screen (for example the header at 60% width is not enough on my 13" laptop) but I'm sure you'll fix that when you make it responsive 😉
|
Thank you very much for the positive comments. Yes, I can fix it. |
|
I am unsure whether I made the pull request correctly! I think I have done my best to fix navigation section and the layout for the cases considering small devices. I have to find a way to make my navigation collapse. Thanks for spending your valuable time on checking my code and giving feedback to improve my knowledge further. |
css/_abstract.scss
Outdated
| @content | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
This seems like overkill Raj, I would try something simpler. If you want to store your breakpoints inside variables, that's good practice. For example:
$max-width-mobile: 767px;
@media (max-width: $max-width-mobile) {
margin-left: 0;
}
this is more semantic than @include respond(phone) {
With @include respond(phone) { I do not know if we are talking about a size smaller than a phone or bigger than a phone. If you use the term max- or min- it's easier to understand what's going on 🙂
There was a problem hiding this comment.
I'm not sure the @if statements are necessary either. Keep it simple!
There was a problem hiding this comment.
For this simple case, it is not necessary.
css/style.css
Outdated
| } | ||
|
|
||
| @media (max-width: 56.25em) { | ||
| .container { |
There was a problem hiding this comment.
I find that using em for breakpoints makes it harder to understand. Consider using pixels which are more predictable and are widely used in media queries.
There was a problem hiding this comment.
I read something about using px and rem in media query in the link: https://zellwk.com/blog/media-query-units/
If you read the following on the page: (2 and 3)
2. User Zooms In
The original idea behind em based sizes was due to older browsers not being able to update pixel values when a user zooms. In this regard, testing the difference between media query units when a user zooms will help to answer the question on whether we can use px based media queries now.
3. User Changed Their Browser’s Font Setting
Concluding The Experiments
As you can see from our tests above, the only unit that performed consistently across all four browsers is em. There aren’t any differences between em and rem with the exception of bugs found on Safari.
px media queries performed well in two of the three experiments (with the exception of Safari, again). Unfortunately, px media queries remained at 400px in the third experiment, which makes it a no-go if you intend to support users who change their browser’s font-size value.
Hence, my conclusion after these experiments is: Use em media queries.
css/style.css
Outdated
| .navigation { | ||
| opacity: 0; | ||
| visibility: hidden; | ||
| } |
There was a problem hiding this comment.
I suppose you did this in anticipation of collapsing your menu? 🙂
There was a problem hiding this comment.
Yes, that is right. I will fix it later.
| .cases__devices { | ||
| flex-direction: column; | ||
| } | ||
| } |
There was a problem hiding this comment.
Good, this looks great!
Watch out for the spacing around that section - there is no padding at the top and around the headline, and a lot of whitespace at the bottom. Try and even the space out.
css/style.css
Outdated
| .cases__content > h2 { | ||
| font-size: 2rem; | ||
| margin: 0; | ||
| } |
There was a problem hiding this comment.
Give this headline some breathing space 🌬
|
I have added more changes to the code. Please have a look at them when you get time. Thanks very much! |
This is my answer for the homework. Please check and let me know if I need to do anything (@LucyMac ). I have not considered responsive design at this moment.