-
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
Kelly Souza Static Site #35
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.
Nice job manipulating images and creating a parallax effect. I echo Kari's comment regarding #post1 being a bit confusing and room for improvement in id/class semantics/usage. Good job going back and getting the alt-text for the images -- something that was suggested to me is to consider what the image is providing to a sighted person that someone using a screen reader would miss out on. Sometimes the image is purely decorative, in which case an empty string may actually be preferred.
|
||
|
||
|
||
#wrap { |
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.
Doesn't appear to be used in html -- could this have been removed?
</main> | ||
</div> | ||
<footer> | ||
<h4> © 2017 - </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.
Are the h4 and p tags styled differently here? Would it have made sense to combine them?
</p> | ||
</section> | ||
|
||
<div id="slide1" class="slide"> |
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.
Would it have been possible to give this div id a more contextual rather than positional name? What if you wanted to swap slide 1 with slide 2 -- would the names still make sense?
</section> | ||
|
||
<div id="slide1" class="slide"> | ||
<div class="title"> |
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.
The text is a little difficult to read -- does this meet accessibility contrast guidelines?
|
||
<div id="slide4" class="slide header"> | ||
<h1>The End</h1> | ||
<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.
The footer on this page appears different from the others (right icons are cut off/run off the right side of the page in Chrome). Should a footer tag be placed inside a main tag?
</main> | ||
|
||
<footer> | ||
<h4 class="copyright-date"> © 2017 - </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.
Is this class being used as a selector somewhere?
index.html
Outdated
</ul> | ||
</nav> | ||
</header> | ||
<div id=wrap> |
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.
Missing ''
portfolio.html
Outdated
</ul> | ||
</nav> | ||
</header> | ||
<div id=wrap> |
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.
Also missing ''
Static Site
Congratulations! You're submitting your assignment!
Comprehension Questions