-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Haby's Static Site #31
base: master
Are you sure you want to change the base?
Conversation
Static SiteWhat We're Looking For
|
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.
Hello Haby! I'm sorry for the delay with my code review! I just added a couple comments that could help dry up your code, but overall great job :). I liked what you did with your social media icon links - neat! Keep going strong!
<body id="page3"> | ||
<header> | ||
<div class="header-name"> | ||
<a href="about.html" target="_blank">Haby Randall</a> |
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.
You might want to rethink using target="_blank" for links to pages of your website since it opens pages into a new tab. Makes sense and super useful for the way you used this for your links to social media sites!
<nav class="top-nav"> | ||
<ul> | ||
<li class="btn"> <a href="hobby-blog.html" target="_blank">Hobbies</a></li> | ||
<li class="btn"> <a href="porfolio.html" target="_blank">Porfolio</a></li> |
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.
link is broken because portfolio is mispelled
<main> | ||
<section> | ||
<article class="bio"> | ||
<img src="images/IMG_4566.JPG" /> |
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.
adding a description under an "alt" attribute to image tags can be helpful in case links to images are broken.
</header> | ||
|
||
<main> | ||
<section> |
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.
section tags may be unnecessary since page is already divided into articles
<main> | ||
<section id="posts"> | ||
<article class="hobby"> | ||
<img src="images/IMG_3095.jpg" alt="puppies"/> |
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.
cute puppy! :) and nice use of alt attribute
|
||
<main> | ||
<section class="introduction"> | ||
<article class="intro-paragraph"> |
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.
looks like your articles have two different classes: "intro-paragraph" and "about-me-paragraph" yet the same styling. So these could be under the same class, but since the classes weren't called in your stylesheet, I wouldn't give them a class at all. If this was this was something you did to label/keep track of your paragraphs, I would use comments instead.
margin-top: 5%; | ||
|
||
} | ||
.github, .instagram, .twitter, .facebook { |
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.
could have named these all one class (i.e. .social-media) instead of separate classes. Class is used like ID here.
color: inherit; | ||
} | ||
|
||
footer { |
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.
Are there some ways css could be dried here? There are similar styles to header.
Static Site
Congratulations! You're submitting your assignment!
Comprehension Questions