Skip to content

Flexlayout karma raj#1

Closed
rajgthub wants to merge 11 commits intoCodeYourFuture:masterfrom
rajgthub:flexlayout_karma_raj
Closed

Flexlayout karma raj#1
rajgthub wants to merge 11 commits intoCodeYourFuture:masterfrom
rajgthub:flexlayout_karma_raj

Conversation

@rajgthub
Copy link

@rajgthub rajgthub commented Apr 1, 2018

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.

Copy link
Collaborator

@LucyMac LucyMac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😉

@rajgthub
Copy link
Author

rajgthub commented Apr 1, 2018

Thank you very much for the positive comments. Yes, I can fix it.

@rajgthub
Copy link
Author

rajgthub commented Apr 1, 2018

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.

@content
};
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure the @if statements are necessary either. Keep it simple!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this simple case, it is not necessary.

css/style.css Outdated
}

@media (max-width: 56.25em) {
.container {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose you did this in anticipation of collapsing your menu? 🙂

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is right. I will fix it later.

.cases__devices {
flex-direction: column;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will add padding.

css/style.css Outdated
.cases__content > h2 {
font-size: 2rem;
margin: 0;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Give this headline some breathing space 🌬

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, surely!

@LucyMac LucyMac added the reviewed A mentor has reviewed this PR label Apr 5, 2018
@rajgthub
Copy link
Author

I have added more changes to the code. Please have a look at them when you get time. Thanks very much!

@LucyMac LucyMac added the London class 4 start March 2018 label Mar 12, 2019
@LucyMac LucyMac closed this Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

London class 4 start March 2018 reviewed A mentor has reviewed this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants