Week3/sadatakhtar #75
Conversation
There was a problem hiding this comment.
Solid website, and good to see a use of js and a lot of media queries to achieve different layouts.
There's a few issues comments about the organisation of your CSS. A lot of the time you could avoid repeating yourself by using classes instead of ID's. Additionally, you've also made it a little bit more difficult for yourself in that it's not commented or structured in an easy way. This means there's repetition within your media queries, and might have led to issues with specificity, as whatever is further down in a css file has more specificity than something further up.
However, you've generally grasped the principles, and challenged yourself by adding more than what was asked so well done.
| box-sizing: border-box; | ||
| } | ||
| body{ | ||
| height: 100vh; |
There was a problem hiding this comment.
this was having scroll issues for me, so at some point you wanted to add overflow: scroll; to the body to allow users to scroll.
Additionally, min-height is more appropriate here, as that prevents the site contents to be constrained to 100vh.
| <!-- Remember: Use semantic HTML tags like <header>, <main>, <nav>, <footer>, <section> etc --> | ||
| <header> | ||
| <nav> | ||
| <div class="logoandtitle"> |
There was a problem hiding this comment.
Semantically, I don't think that the logo is part of the navigation. So it might not be correct to have it as a child of the header.
| @@ -0,0 +1,27 @@ | |||
| //selectors | |||
There was a problem hiding this comment.
Looking good, nice to see some js in here.
| background-image: url(./images/cake-image.png); | ||
| background-repeat: no-repeat; | ||
| background-position: center bottom; | ||
| background-size: cover; |
There was a problem hiding this comment.
When using background cover, this will obscure parts of the image to fit the container. While this is fine for most images, when you have text inside an image, this can be hidden at certain widths:
| left: 10%; | ||
| z-index: -1; | ||
| } | ||
| #ellipse2{ |
There was a problem hiding this comment.
As you're sharing css properties across several IDs' You could have used one class of ellipse instead of writing out the same position and z-index for every ellipse.
| } | ||
| } | ||
|
|
||
| .line1, .line2, .line3{ |
There was a problem hiding this comment.
You could use comments here to denote that this specific section of your CSS is for the hamburger menu.
| bottom: -50%; | ||
| right: 0%; | ||
| z-index: -1; | ||
| }.mainsection{ |
| background-size: cover; | ||
| } | ||
| .welcome{ | ||
| font-size: 20px; |
There was a problem hiding this comment.
We'd generally use EM or REM for font-size and pixels heights can cause issues for accessibility.
| .hero{ | ||
| height: 80vh; | ||
| } | ||
| /* .mainsection{ |
There was a problem hiding this comment.
Unless there's a good reason, you'd want to remove commented out code from any pull requests into master.
| background-image: url(./images/cakehero2.jpg); | ||
| background-repeat: no-repeat; | ||
| background-position: center bottom; | ||
| background-size: cover; |
There was a problem hiding this comment.
As some of these properties are already present earlier in the file, you don't need to repeat yourself for this media query.
I have adjusted media queries to fit all the sizes of mobile devices available in the debugging console.