some changes done#2
Conversation
|
Hi Lucy, |
| <li>Login</li> | ||
| </ul> | ||
| <img class="logo" src="img/karma-logo.svg" width="26"> | ||
| </div> |
There was a problem hiding this comment.
if the logo appears first, from left-to-right, then it should (in most cases) appear first in your HTML.
Try moving it before the <ul> and you won't need the negative margin anymore: https://github.com/CodeYourFuture/karma-clone/pull/2/files#diff-6a6a912d0c7b55b537768da778032849R44
There was a problem hiding this comment.
Also, try adding display: flex; to the .content div and position the logo and nav links using flexbox 😉
| </section> | ||
| <section class="cases"> | ||
| <div class="content"> | ||
| <h2>Everyone needs a little Karma.</h2> |
There was a problem hiding this comment.
Again, try adding display: flex to the .content container and position the list items that way.
To avoid the devices spreading too wide, contain them with a max-width
| border: 1px solid #eaebec; | ||
| padding: 0.625rem 0; | ||
| text-align: center; | ||
| } |
There was a problem hiding this comment.
To make your circles perfectly round, you need 2 things:
- the border-radius at 100%
- equal padding all around the inside (currently only top and bottom)
| @@ -8,9 +8,80 @@ | |||
| <link rel="stylesheet" href="css/normalize.css"> | |||
There was a problem hiding this comment.
Add this line in your <head>:
<meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no">
It will ensure that the page is scaled on a mobile. At the moment, everything is tiny 🔍
There was a problem hiding this comment.
With this, and the use of flexbox on your .content, as suggested, you will find it much easier to position your content on mobile 👍
|
Hi Lucy, |
| .intro { | ||
| height: auto; | ||
| width: auto; | ||
| } |
There was a problem hiding this comment.
The height of this section as initially almost 46rem and is now set to auto. Auto means it will only be as big as the content. There isn't much content (headline + button) so the hero is suddenly very small! You can fix this by adding padding to .intro, it will make it grow.
https://github.com/CodeYourFuture/karma-clone/pull/2/files#diff-6a6a912d0c7b55b537768da778032849R54
| padding: 0 0.938rem; | ||
| margin: 0 auto; | ||
| min-width: 37.5rem; | ||
| } |
There was a problem hiding this comment.
This is too much for mobile, change it in your media query. If you want it to adapt to the size of the device you can say min-width: 100%
Tip: min-width has more specificity than width so min-width (and max-width) will always override width.
| width: 33.3333333%; | ||
| float: left; | ||
| display: flex; | ||
| justify-content: space-around; |
There was a problem hiding this comment.
float: left has no effect if you are using Flexbox. Either you use floats, or you use flexbox. For this exercise, stick to flexbox 🙂
Once you get used to it, Flexbox is much easier to control and gives you more flexibility for positioning.
| display: flex; | ||
| flex-direction: column; | ||
| align-items: flex-start; | ||
| } |
There was a problem hiding this comment.
would you not want to center the column?
| } | ||
| .cases .content { | ||
| margin-top: 5rem; | ||
| } |
There was a problem hiding this comment.
This is a very big margin for a small screen.
| border-radius: 100%; | ||
| width: 1.5rem; | ||
| height: 1.5rem; | ||
| display: inline-block; |
There was a problem hiding this comment.
only display: flex please!
You will see it will be much easier to center the icons inside the bubbles with flexbox
| } | ||
| .navigation .links { | ||
| display: flex | ||
| } |
There was a problem hiding this comment.
on mobile the navigation doesn't fit on the screen. Try to think of a way to fix this (make it smaller? tighter? or disappear completely?)
LucyMac
left a comment
LucyMac
left a comment
There was a problem hiding this comment.
It's better Than Than, but the size of elements on mobile still needs adjusting. The 3 cases blocks are better centered but they are too small. Let them grow and use up the space.
Also the padding at the bottom of the page is too large on mobile.
The problem is you only have 1 mobile breakpoint (600px) when you probably need a couple more to fine-tune everything.
A few adjustments, but otherwise much better.
| </div> | ||
| </nav> | ||
| <section class="intro"> | ||
| <div class="intro"> |
There was a problem hiding this comment.
Watch out, you have 2 instances of the .intro class so the background image is appearing twice.
2 participants
Please drop comment!