Html css mb week2 #62
Conversation
There was a problem hiding this comment.
Really good work.
Added a couple of pointers regarding classnames, positioning, and other nitpicks.
Also, be careful about formatting your code and removing any unnecessary comments before opening a PR.
Also - there seems to be a lot of extra files committed. Maybe check with your teammates to see what way they did their PRs.
| <body> | ||
|
|
||
| <header class="nav-header"> |
There was a problem hiding this comment.
I really think a container would have helped you here.
By having a container - it allows you to give the header element a max-width of say 650px
This would allow you to remove the percentage widths of the logo and nav children and just use justify-content: space-between
You'll notice if you pull in the window your nav items start to stack before all the space is taken up because they are restricted to stay within 60% of the window.
Alternatively you could make the the body a flex box - but I don't encourage this.
| <body> | ||
|
|
||
| <header class="nav-header"> | ||
| <div class="logo"> | ||
| <img src="/img/karma-logo.svg" alt="karma logo" width=42 height=42> |
There was a problem hiding this comment.
| <img src="/img/karma-logo.svg" alt="karma logo" width=42 height=42> | |
| <img src="/img/karma-logo.svg" alt="karma logo" width="42" height="42"> |
all html attributes should be enclosed in double quotes
| </header> | ||
| <section class="image-container"> | ||
| <div class="image-section"> | ||
| <p class="big-writing">Introducing Karma</p> |
There was a problem hiding this comment.
Nice use of reusable classes :)
FYI - It's common to shorten size notation with sm, md, lg then combine for finer tunes like sm-md md-lg and also using x for extra - xs xl
Also I think font is probably more programmatically correct term instead of writing
These are all nitpicks by the way - not really important - but good to get used to universal standards
| <p class="inc">Karma Mobility, Inc</p> | ||
| </footer> | ||
|
|
||
|
|
There was a problem hiding this comment.
Always remove any comments that are not your own - or comments that no longer make sense in the context of the work being submitted
| <!-- Add your HTML markup here --> | ||
| <!-- Remember: Use semantic HTML tags like <header>, <main>, <nav>, <footer>, <section> etc --> | ||
| <!-- All the images you need are in the 'img' folder --> | ||
|
|
||
| </body> | ||
| </html> | ||
|
|
||
| </html> |
There was a problem hiding this comment.
indentation is a little out of whack throughout the file... It's good to make sure your code is correctly formatted before submission. You can find auto formatters on vscode for any language. Some will do it every time you save so you never have to think about it again :)
Again just a nitpick but becomes really important as you move forward.
|
|
||
| } | ||
| .image-container p { | ||
| display: flex; |
There was a problem hiding this comment.
None of these attributes seem to be doing anything.
You generally wouldn't be giving text tags a display of flex.
I think you can remove this block altogether.
Also FYI - It's good practice to remove any commented code before submitting a PR
|
|
||
| } | ||
|
|
||
| #button { |
There was a problem hiding this comment.
I would prefer this to be a class.
It's fine for now - being the only button...
But you can imagine this button featuring elsewhere if you were to start doing the other pages on the navbar.
You could of course give them all an id of button but this is generally bad practice. IDs should be unique.
Use classes for styling where possible
| padding-bottom: 8rem; | ||
|
|
||
| border-bottom: 1px solid rgb(226, 225, 225); | ||
| margin-left: auto; |
There was a problem hiding this comment.
Would prefer to use flex to center.
Remove margins and use
justify-content: center;
width: 100%;
and on the child elements give
margin: 0 20px;
| #facebook { | ||
| font-size: 1rem; | ||
| color: #4267B2 | ||
| } |
There was a problem hiding this comment.
Can format this also ;)
Try ctrl+a then right click and see if there is an option to format document
|
Also - forgot to mention - the image urls are not working as expected. |
|
Thanks Rody. I had a feeling some code wasn’t really doing anything but I thought Ill just leave it just in case I break it somewhere else :)
… On Jun 4, 2020, at 5:32 PM, Rody-Kirwan ***@***.***> wrote:
Also - forgot to mention - the image urls are not working as expected.
Remove the first forward slash
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#62 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APHIZ4TGVR4MHEMJWBLMQITRU7ECXANCNFSM4NIFITQQ>.
|
Week 2s homework Martin Boylan