-
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
sai's static site #28
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.
Hi! This is a very pleasing site, with pretty good responsiveness, and it was fun to read. I left a few comments on some specific lines of code, but in general:
- Great job with relative units!
- I liked that you were willing to try a lot of different CSS strategies, like pseudoclasses.
- There are some divs which could be avoided by styling more semantic html tags directly (h1s and other header tags, nav, etc)
- Some of the classes you've declared in the html are never utilized in the CSS and/or are only used once.
(Also, on a note that's totally unrelated to code, I love that you listed sociolinguistics as an interest. As someone who minored in linguistics in college and loved her sociolinguistics course, I'm excited there are other linguistics nerds at Ada :) )
text-decoration: none; | ||
border-bottom: dotted; | ||
border-width: thin; | ||
border-color: #f7ce3e |
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 found it a little difficult to see the underlined links here. Have you discussed accessibility yet? There are a lot of neat tools out there for checking things like color contrast. Better color contrast make pages easier to read/interpret for people with vision impairments such as color blindness (or, in my case, tired eyes ;) ). A great site is
the WebAIM Color Contrast checker. Hope this helps! :) I do really love that you used the border property to underline the links, though; it makes for a nice effect!
(Another minor thing to note here: you're missing a semicolon at the end of line 12.)
padding-left: 2%; | ||
} | ||
|
||
article.project { |
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.
Great job on using percentages wherever possible! Relative units are an easy way to ensure responsive sites; your site is quite responsive and transitions between differently-sized windows pretty smoothly for the most part.
<div class="hello"> | ||
<h1>< hello. ></h1> | ||
</div> | ||
<div class="nav"> |
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.
Why have a class called .nav
? Could you avoid the div
and style the more semantic <nav>
tag directly?
padding-left: 6%; | ||
} | ||
|
||
.hello, .disclaim { |
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.
It seems as if you are using the .hello
class to style the title of your site. Might you style the <h1>
on each page directly instead?
vertical-align: top; | ||
} | ||
|
||
article.project p:first-of-type { |
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.
While this is a creative use of the first-of-type
pseudoclass, I wonder if it's really necessary in this case. What else could you use instead of a <p>
tag as headers for your sections?
<p>I maintain a very spotty social media presence across too many platforms:</p> | ||
<ul class = "social-media"> | ||
<li><a href="https://twitter.com/saisamant", target = "_blank"><img src="assets/twitter.svg" alt="twitter" height="40px"></a></li> | ||
<li><a id="ravelry" href="http://www.ravelry.com/people/samtheant", target = "_blank"><img src="assets/ravelry.svg" alt="knit" height="40px"></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.
I feel like your alt
attributes could be more descriptive here. Remember that the main purpose of an alt
description is to accurately describe what the image is, both for screen readers and in case the image fails to load. Does "knit" accurately describe this image, or would "ravelry logo" suit better? Similarly for "insta" and "fb", as well as "git" on your code page.
<main> | ||
<section class="all-interests"> | ||
<article class="interest"> | ||
<article class="podcasts"> |
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 never seem to use any of these classes ("podcasts", "knitting", "sociolinguistics", etc) in your CSS or elsewhere. What purpose do they serve, and why do each of your interests have their own class?
Static Site
Congratulations! You're submitting your assignment!
Comprehension Questions
tags and in come cases I don't have content, so I just need to figure out how to update. Also it suggested adding a lang = english tag at the top.