-
-
Notifications
You must be signed in to change notification settings - Fork 43
JiT update.... #64
base: master
Are you sure you want to change the base?
JiT update.... #64
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work Leroy.
left a couple of small notes but otherwise really good.
week2/2-website/index.html
Outdated
<button>Learrn More</button> | ||
</div> | ||
</div> | ||
<main id=benefits> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<main id=benefits> | |
<main id="benefits"> |
missing double quotations :)
week2/2-website/index.html
Outdated
</div> | ||
</main> | ||
<footer> | ||
<h4></h4> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the empty h4
week2/2-website/css/style.css
Outdated
@media (min-width: 76.8em) { | ||
.content-container { | ||
width: 73.6em; | ||
sbackground-color: red; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems to be a typo here
sbackground-color: red; | |
background-color: red; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Leroy uses this instead of commenting.
week2/2-website/css/style.css
Outdated
@media (min-width: 99.2em) { | ||
.content-container { | ||
width: 96rem; | ||
sbackground-color: blue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
week2/2-website/css/style.css
Outdated
@media (min-width: 120rem) { | ||
.content-container { | ||
width: 60%; | ||
sbackground-color: green; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
week2/2-website/css/style.css
Outdated
color: #3f729b; | ||
} | ||
|
||
.fa{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to create your own classname rather than setting styles directly on fontawsome class.
Consider that later you might want to have a bigger icon which will mean you'll have to overwrite this. and then reset it again for smaller icons. Maybe consider creating classes like
icon-sm
icon-md
and icon-lg
.
Then you can do something like
<i class="fa fa-twitter icon-sm">
Also I'm not sure you need to set the font-family
week2/2-website/css/style.css
Outdated
} | ||
|
||
nav.menu { | ||
/* float: right; */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove any unnecessary / commented code before submitting a PR.
Removed unecessary code and comments. Also used the proper class to size the social media icons.
No description provided.